Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase compiler warning levels and associated code changes #1

Merged
merged 1 commit into from
May 27, 2015

Conversation

benalexau
Copy link
Contributor

Thanks for Better Enums. It's incredibly useful.

Some projects wishing to include enum.h might be following the advice of enabling all available compiler warning flags (eg C++ Coding Standards by Herb Sutter and Andrei Alexandrescu, Item #1, "Compile cleanly at high warning levels"). Such projects currently need to either modify enum.h or use #pragm directives to disable warnings for that file.

This PR modifies the example and test Makefile to enable all available warning flags, along with associated code changes to comply with the resulting compiler warnings. This allows enum.h to be directly used in projects without editing or #pragma directives. The test cases pass and the examples execute without error.

@aantron
Copy link
Owner

aantron commented May 27, 2015

Hi, Ben, and thank you for the first pull request. It's good to see the library getting usage :)

I'm strongly in favor of what you have done. I had actually forgotten that there are many flags not included in -Wall.

At the moment, I have a messy topic branch that is considerably different from master. I haven't merged any of the changes into master for a while because they result in non-trivial divergence from the documentation. I am still new to GitHub. What's the right thing to do in this case? Apply the changes myself to the extent possible, and credit you with git commit --author? I do have a private devel branch that could be published, but the topic branch that I'm working on is yet another branch. It rewrites a lot of the same files as this pull request. I wouldn't want to show it in its current state, as it has an extremely messy commit history that I intend to squash.

Thanks again!

@benalexau
Copy link
Contributor Author

If you're happy with the changes, I'd suggest to merge this PR into the current master so other users can obtain a more warning level friendly enum.h that remains compatible with the current documentation. This also lets you more easily merge those changes you wish to preserve into your local topic branch. Adding the extra compiler flags to your local topic branch will be useful as it will highlight warning flags etc as you go about your refactoring.

@aantron
Copy link
Owner

aantron commented May 27, 2015

Thanks. Let me give them a proper review, especially the #pragma.

I am indeed now planning to more meticulously enable warning flags for each compiler in my topic branch.

@@ -217,6 +217,7 @@ struct optional {

namespace _enum {

#pragma GCC diagnostic ignored "-Weffc++"
Copy link
Owner

Choose a reason for hiding this comment

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

This is caused by a mistake on my part. operator = should be "normal", i.e. have return type const _eat_assign& and return *this.

@aantron
Copy link
Owner

aantron commented May 27, 2015

If you don't mind, I will merge the edits you made to silence the warnings into master, except for the #pragma, and make sure credit on the commit is to you. Is this the right thing to do, or is the custom to have the submitter always update the pull request? In the latter case, I think this should be squashed first.

@benalexau benalexau force-pushed the master branch 2 times, most recently from 8f79edf to d45405e Compare May 27, 2015 04:09
@benalexau
Copy link
Contributor Author

@aantron, thanks. I have made the changes you requested (ie removed Makefile warning flag additions, squashed commits, avoided use of #pragma in enum.h). If you can take a look at the updated commit, I am happy to make further changes to assist.

@aantron
Copy link
Owner

aantron commented May 27, 2015

@benalexau, thanks – it looks good. The only request I have is that you change the commit message, since this now doesn't add any flags, just makes sure the code will compile if people do add them.

These modifications ensure enum.h can be used in a wider
selection of end user projects without triggering warnings.

GCC 4.9.2 was used with the following warning flags set:

-Wall -Wextra -Wshadow -Weffc++ -Wno-unused-parameter
-Wno-unused-local-typedefs -Wno-long-long -Wstrict-aliasing
-Werror -pedantic -std=c++1y -Wformat=2 -Wmissing-include-dirs
-Wsync-nand -Wuninitialized -Wconditionally-supported -Wconversion
-Wuseless-cast -Wzero-as-null-pointer-constant

This commit includes the modifications required to enable successful
use of enum.h via both the "test" and "example" directories.
@benalexau
Copy link
Contributor Author

Commit message modified.

aantron added a commit that referenced this pull request May 27, 2015
Modifications to support aggressive compiler warning levels.
@aantron aantron merged commit dd4c116 into aantron:master May 27, 2015
@aantron
Copy link
Owner

aantron commented May 27, 2015

Thank you :)

@benalexau
Copy link
Contributor Author

Congratulations on your first pull request cycle. I hope there will be many more! :-)

@aantron
Copy link
Owner

aantron commented May 27, 2015

@benalexau thanks for helping me with it :) I have one last question – I tried the web merging UI despite reservations and now I have your commit coming off master and then getting merged right back into it in the history. Is that what people want to see, or this is a problem of GitHub? I prefer to turn this into a straight line and push -f the fixed master.

@benalexau
Copy link
Contributor Author

Normally the "merge" message gets its own commit. I personally agree it looks ugly. You also get these a lot with even git pull if you don't use --rebase. I see you have done a push -f already to fix it up, though. :-)

@aantron
Copy link
Owner

aantron commented May 27, 2015

Yeah, I'm going to shoot for having a "clean" history for as long as possible :) Thanks for all your help. Have a good whatever time of day it is, guessing morning :)

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.

None yet

2 participants