Skip to content

Small changes#6

Merged
ppescher merged 1 commit into
ppescher:masterfrom
irwir:updates
Jan 3, 2018
Merged

Small changes#6
ppescher merged 1 commit into
ppescher:masterfrom
irwir:updates

Conversation

@irwir

@irwir irwir commented Jan 1, 2018

Copy link
Copy Markdown
Contributor

Prefer default initialization.
Remove unused and predefined preprocessor macros.
Add /Zc:throwingNew option.

Remove unused and predefined preprocessor macros.
Add /Zc:throwingNew option.

@ppescher ppescher left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, but why is the project file involved?

@irwir

irwir commented Jan 3, 2018

Copy link
Copy Markdown
Contributor Author

Project file changed as a side effect of general clean-up. This is not mandatory for the other changes in this pull request, but still an improvement.

Github's online diff tool is awful, it shows the whole file was messed; Visual Studio's internal compare is better.
For each build configuration only one line changed and one line added.

I assume some macro definitions remained from the days before Visual C++ 6.0.
Now, there is no need to define WIN32, _WINDOWS and _DEBUG. The last one is predefined, the first two are never used anywhere. Also there are predefined alternatives such as _WIN32.

The option /Zc:throwingNew forces C++ standard behaviour for new operator, which is throw instead of returning a null pointer.

@ppescher

ppescher commented Jan 3, 2018

Copy link
Copy Markdown
Owner

Yes the project files have been converted from VC++6.0 first time on Visual Studio 2008.
So many things have changed since then that maybe it's worth it to re-create all the projects with recent IDE versions, so I get default build options that are more up to date.
I may also drop non-unicode builds, since I don't (and I couldn't anyway) support Win9x anymore.

@irwir

irwir commented Jan 3, 2018

Copy link
Copy Markdown
Contributor Author

The latest VS still does non-Unicode builds.
Perhaps the library could keep non-Unicode versions while no additional efforts or code changes were required,

@ppescher

ppescher commented Jan 3, 2018

Copy link
Copy Markdown
Owner

I guess project defaults now have Unicode enabled, so having Debug|Release build configurations that are MBCS, and custom configurations for Unicode, might be anachronistic or just misleading. Maybe it should be the other way round, that's why I thought I should re-create the projects using the latest IDE, so I get default behavior.

@irwir

irwir commented Jan 3, 2018

Copy link
Copy Markdown
Contributor Author

Therea are ANSI builds also.
My idea is to keep things compatible in any possible way while it goes for free.

@ppescher

ppescher commented Jan 3, 2018

Copy link
Copy Markdown
Owner

Sure, I don't want to break compatibility. Just "refreshing" the project files so they are more suitable for todays IDE's default options.

@ppescher

ppescher commented Jan 3, 2018

Copy link
Copy Markdown
Owner

By the way, how do you specify ANSI builds with MFC support?

@ppescher

ppescher commented Jan 3, 2018

Copy link
Copy Markdown
Owner

I guess I had messed up build options and mistakenly substitute MBCS for ANSI builds when I first converted the project files. Now I have a somewhat "bad" release 1.5 that I feel I should either fix or retire.

@irwir

irwir commented Jan 3, 2018

Copy link
Copy Markdown
Contributor Author

It seems you already figured out that ANSI character set is the default. It is either Not set or an empty line in Properties dialog.
MBCS is obsolete now, and Unicode should be prefered instead.
Possibly you could create version 1.5.1 with the latest additions and projects being fixed.

@ppescher ppescher merged commit 29db089 into ppescher:master Jan 3, 2018
ppescher added a commit that referenced this pull request Jan 3, 2018
Prefer default initialization.
Remove unused and predefined preprocessor macros.
ppescher added a commit that referenced this pull request Jan 3, 2018
Prefer default initialization.
@ppescher

ppescher commented Jan 3, 2018

Copy link
Copy Markdown
Owner

I had a look at default project options and finally decided to merge without /Zc:throwingNew and the other project file changes. It seems that project wizard still adds those preprocessor macros.

@irwir

irwir commented Jan 4, 2018

Copy link
Copy Markdown
Contributor Author

Thanks for merging.

It could be an apprentice project wizard. :)
The wizard makes a template for further changes, not a finished product..
There are many more urgent issues in VS than taking care of the wizard, which was not badly broken yet. No big deal if it adds some useless, and maybe undocumented, macros.

The compiler option in question was recommended in VC++ blog: https://blogs.msdn.microsoft.com/vcblog/2015/08/06/new-in-vs-2015-zcthrowingnew/
Earlier VC++ versions warn about unknown option, but nothing more.

For a more lightweight project you could delete the lines
<CharacterSet>Not set</CharacterSet>
It still would be an ANSI build.

This was not for reopening this pull request, but explanation of the reasons behind the changes.

@ppescher

ppescher commented Jan 4, 2018

Copy link
Copy Markdown
Owner

Thanks irwir for the explanation and the reference.

I feel like I have many things to catch up with about Windows desktop programming. Now most of my work is with embedded systems (microcontrollers) and my Windows programming is a bit rusty.

I had a look at that MSDN post and I must say I agree with many of the comments: that option should have been documented (1.5 years later nothing changed) and now added to the new IDE (VS2017). It should have also become the default for new projects, but since it is still undocumented of course that could not happen. Go figure why...

I left out those changes to project files because I want to fix the last release, and introduce only strictly required changes. I anticipate to refresh all the projects in the future, so I'll make those changes afterwards.

I would like to provide builds for ANSI (probably as a sort of "legacy" or "compatibility" option only, maybe a different project?) and Unicode, as well as x86/x64. I would gladly hear any suggestions about the proper way to do that. I don't really like the idea of ending up with "Debug Static Unicode x64" and so on... do you know of a better way?

@irwir

irwir commented Jan 5, 2018

Copy link
Copy Markdown
Contributor Author

To make sure it was not missed, Github sent 4 copies of the latest message to me.

"Debug Static Unicode x64"

Platform is taken care automatically, no need for x64; though you did not add that part already.

It is possible to leave only generic configurations and let users to adjust as required (change platform and character set); but I am not sure that would be a better option.
Unfortunately, no great way to name configurations exists; the names are either long and descriptive, or short and unclear.

Use of macros such as $(Platform)$(Configuration) might be preferred to explicit directory names such as .\Release - that is what VS does in new projects.
Also you might change resources' culture from Italian 0x410 to neutral.

@ppescher

ppescher commented Jan 5, 2018

Copy link
Copy Markdown
Owner

Yes, that is also planned (but I think it will come very slowly now that I ended my holidays).
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants