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

Conditionally disabled -Weffc++ and -Wshadow for compilers with broken implementations #945

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rjk9w5
Copy link

@rjk9w5 rjk9w5 commented Oct 6, 2020

Ref. #937 This will turn of these flags which trigger false positives in GNU and Intel compilers.

…d -Wshadow for compilers with broken implementations
@TedLyngmo
Copy link
Contributor

I don't know CMake very well, but instead of turning -Weffc++ off completely for those compilers, would it be possible to make it disable the warning only for the line for which it gives a false positive so that the result becomes something like the below?

include/yaml-cpp/node/impl.h

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Weffc++"
  return *this;                             // this is currently line 206
#pragma GCC diagnostic pop

@rjk9w5
Copy link
Author

rjk9w5 commented Oct 7, 2020

For sure that can be done. The benefit of this approach is it is less invasive to the code. In general I am not a huge fan of polluting the code base with compiler specific pragmas. It also doesn't guarantee in the future when things are added people will remember to add the pragmas.

The scope of bring these into compliance is rather larger than makes sense for flags that aren't really that important. With -Wall -Wextra -pedantic -pedantic-errors it is already pretty strict. -Wshadow is only disable for Intel because it can't do scope checking correctly

Here are just a couple from Intel 2019. It is possible, but really gross, to put pragmas around all of the locations it is needed. Some of them could be fixed with #pragma once instead of using the generated header guards, but many of them are things that aren't even really in the guide...

Fun
-Weffc++

../include/yaml-cpp/null.h(2): warning #2012: Effective C++ Item 1 prefer const and inline to #define
  #define NULL_H_62B23520_7C8E_11DE_8A39_0800200C9A66

This isn't even item 4, not sure what they are doing
-Weffc++

../include/yaml-cpp/eventhandler.h(38): warning #2015: Effective C++ Item 4 prefer C++ style comments
    virtual void OnAnchor(const Mark& /*mark*/,

static member functions are not in the same scope as non-static member variables...
-Wshadow

../include/yaml-cpp/exceptions.h(165): warning #3868: declaration hides member "YAML::Exception::mark" (declared at line 161) in same scope
    static const std::string build_what(const Mark& mark,

@TedLyngmo
Copy link
Contributor

I agree that adding the compiler specific pragma directly into the code isn't nice. I was thining of tagging it somehow so that CMake could turn it into whatever the compiler needs.

Perhaps -Weffc++ is both a bit broken and perhaps the advice it gives is a bit outdated. I added it because it helped me to find some minor initialization bug if I remember correctly and I thought it would be good to keep it in, but perhaps it should be removed completely instead?

@rjk9w5
Copy link
Author

rjk9w5 commented Oct 8, 2020

Well this patch only turns it off for compilers with incorrect implementations. The clang implementation works great, and I agree when it is working correctly it can be very beneficial. The CMake change is just disabling it where the compiler is reporting more false positives than catching actual useful errors.

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