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

Mark include directories of 3rd-party libraries as system includes #2733

Merged
merged 1 commit into from Dec 27, 2018

Conversation

SunBlack
Copy link
Contributor

Not all include directories of 3rd-party were marked as system include directories. Fixed it with this commit.

This should fix warnings like this

/usr/include/vtk-6.2/vtkMath.h:672:3: warning: multi-line comment [-Wcomment]
   // a & b \\
   ^
/usr/include/vtk-6.2/vtkMath.h:676:3: warning: multi-line comment [-Wcomment]
   // 1 & 0 \\
   ^
/usr/include/vtk-6.2/vtkMath.h:679:3: warning: multi-line comment [-Wcomment]
   // a & b \\
   ^

After this PR it should be possible to add sth. like this (starting with a lot of exceptions in PCL ;) ), to have a stronger check for new code in upcoming PR.

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Werror")

@SunBlack SunBlack force-pushed the system_includes branch 2 times, most recently from 21d6a98 to ce6cd2d Compare December 22, 2018 00:31
@taketwo
Copy link
Member

taketwo commented Dec 22, 2018

This is a good idea indeed, thanks! However, there are still plenty of warnings from VTK, e.g.:

2018-12-22T02:33:29.5920584Z [ 23%] Building CXX object visualization/CMakeFiles/pcl_visualization.dir/src/pcl_visualizer.cpp.o
2018-12-22T02:33:32.6091530Z In file included from /__w/1/s/visualization/include/pcl/visualization/impl/pcl_visualizer.hpp:48:0,
2018-12-22T02:33:32.6091709Z                  from /__w/1/s/visualization/include/pcl/visualization/pcl_visualizer.h:2347,
2018-12-22T02:33:32.6091845Z                  from /__w/1/s/visualization/src/pcl_visualizer.cpp:100:
2018-12-22T02:33:32.6133715Z /usr/include/vtk-6.2/vtkMath.h:672:3: warning: multi-line comment [-Wcomment]
2018-12-22T02:33:32.6134183Z    // a & b \\
2018-12-22T02:33:32.6134422Z    ^
2018-12-22T02:33:32.6135433Z /usr/include/vtk-6.2/vtkMath.h:676:3: warning: multi-line comment [-Wcomment]
2018-12-22T02:33:32.6135863Z    // 1 & 0 \\
2018-12-22T02:33:32.6135931Z    ^
2018-12-22T02:33:32.6136473Z /usr/include/vtk-6.2/vtkMath.h:679:3: warning: multi-line comment [-Wcomment]
2018-12-22T02:33:32.6136544Z    // a & b \\
2018-12-22T02:33:32.6136592Z    ^

and also from "surface/src/3rdparty/opennurbs/".

@SunBlack
Copy link
Contributor Author

SunBlack commented Dec 22, 2018

This is a good idea indeed, thanks! However, there are still plenty of warnings from VTK, e.g.:

I ran yesterday a build with -Wall -Wextra. Don't ask how many errors I got from CUDA (3984 xD) and I have no idea how to mark CUDA includes as system includes (in our project we use a modern CMake way to include CUDA, but this requires CMake 3.9). Even boost complains (118 errors) - hope with #2732 i can fix it, but this changes are more complex. So this PR just has easy fixes. I hope this time I caught all VTK includes.

and also from "surface/src/3rdparty/opennurbs/".

I can't exclude non header code from check. Only way: create a library with 3rd-party code and disable all warnings manually for this project.

@taketwo
Copy link
Member

taketwo commented Dec 22, 2018

Create a library with 3rd-party code and disable all warnings manually for this project.

I think it's a reasonable approach to create a small static library for each of the bundled 3rd party dependencies and then link it into a correspondent PCL module.

@SunBlack
Copy link
Contributor Author

I think it's a reasonable approach to create a small static library for each of the bundled 3rd party dependencies and then link it into a correspondent PCL module.

Maybe I do this next year. I think I will shutdown my dev PC remotely today or tomorrow (just still running in case I have to adjust a PR ;) ), so I don't do anything big anymore this year.

@taketwo
Copy link
Member

taketwo commented Dec 22, 2018

Still some VTK warnings left...

Maybe I do this next year. I think I will shutdown my dev PC remotely today or tomorrow (just still running in case I have to adjust a PR ;) ), so I don't do anything big anymore this year.

I've done a lot lately. Have a good break and recharge your batteries!

…o prevent compiler warnings which are not caused by PCL code
@SunBlack
Copy link
Contributor Author

Seems I caught now everything of VTK, so this PR is ready to merge :). Boost and CUDA will be in a separate PR, because Boost will be fixed with switch to importet target and CUDA I have currently no idea 😢 So I'm ready for a break 😄

I've done a lot lately. Have a good break and recharge your batteries!

Thx, you too :)

@taketwo
Copy link
Member

taketwo commented Dec 27, 2018

I've done a lot lately. Have a good break and recharge your batteries!

Thx, you too :)

Ooops, of course I meant to write "YOU've done a lot lately" 😅

@taketwo taketwo removed the needs: more work Specify why not closed/merged yet label Dec 27, 2018
@taketwo taketwo merged commit 26901cf into PointCloudLibrary:master Dec 27, 2018
@SunBlack SunBlack deleted the system_includes branch December 27, 2018 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants