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

Treat compiler warnings as errors #3379

Closed
wants to merge 1 commit into from

Conversation

kunaltyagi
Copy link
Member

@kunaltyagi kunaltyagi commented Sep 27, 2019

This ensures that commits with warnings do not get a pass through CI.

Some commits have been already merged. This will track other warnings based on issues/PR

Current status per CI:

  • Format
  • MacOS Mojave
  • MacOS Catalina
  • Ubuntu 16.04
  • Ubuntu 19.10
  • Windows 32-bit
  • Windows 64-bit
  • Documentation: WIP by @aPonza

Current status of modules:

Other related problems(?):

PS: This should be draft or normal PR? Draft

/fixes #2746

@taketwo
Copy link
Member

taketwo commented Sep 27, 2019

PS: This should be draft or normal PR?

I think this is a draft: requires discussion, is not mean to be merged.

I moved Nurbs to the "Pull requests" section in your list since there is a PR in progress by Sergio.

@taketwo
Copy link
Member

taketwo commented Sep 27, 2019

This ensures that commits with warnings do not get a pass through CI.

In reality, this change does not affect CI runs. In the CI config, we set several CXX flags explicitly, which prevents CMake from entering into the branch on line 112. Therefore, -Werror is not added on CI.

Let us add -Werror directly to the CI config and not add it to CMakeLists. The reason for the latter is that there are so many various versions of compilers in the wild that some may emit a warning somewhere. This will result in a failed build, unnecessary confusion for a user, and noise in the bug tracker.

@kunaltyagi
Copy link
Member Author

Let us add -Werror directly to the CI config

This makes more sense.

@taketwo
Copy link
Member

taketwo commented Sep 27, 2019

It took 20s for MacOS job to fail the build 😆

@kunaltyagi
Copy link
Member Author

It took 20s for MacOS job to fail the build

The error is quite unexpected. Do check it

In file included from In file included from /Users/vsts/agent/2.155.1/work/1/s/common/src/pcl_base.cpp:/Users/vsts/agent/2.155.1/work/1/s/common/src/point_types.cpp39::
37In file included from :
In file included from /Users/vsts/agent/2.155.1/work/1/s/common/include/pcl/point_types.h:42:
In file included from /Users/vsts/agent/2.155.1/work/1/s/common/include/pcl/pcl_macros.h:61:
/Applications/Xcode_10.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/cmath:313:9: error: no member named 'signbit' in the global namespace
using ::signbit;

@taketwo
Copy link
Member

taketwo commented Nov 20, 2019

After merging #3477, the Ubuntu 16.04 job will be clean of warnings. I propose to use this opportunity to enable -Wall at least in that job. We'll continue to track progress about making the rest of our jobs warning-free in this PR.

@kunaltyagi
Copy link
Member Author

For 19.04, the warnings are mainly due to memset and memcpy. I might be able to remove some of the memset warnings soon. I tried to make some types trivial in PCL, but that experiment didn't go well. Just writing this to keep track

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