Fix compilation warnings #1019

de-vri-es · 2016-06-03T08:45:57Z

This PR fixes compilation warnings (all of them with gcc 6.1.1 20160501).

I was a bit confused by the existence of setUserData but the absence of getUserData. It seems like a pointless member functions without a matching getter. I changed setUserData to not use the deprecated Ogre::MoveableObject::SetUserAny, and when I wanted to update the getter I noticed it wasn't there.

de-vri-es · 2016-06-08T11:32:23Z

Note that all jenkin builds are marked unstable until the warnings are fixed. I think that policy is fine, but then there should be no warnings on the devel branch (which this PR addresses).

wjwwood · 2016-06-10T01:38:55Z

CMakeLists.txt

@@ -204,7 +204,7 @@ include_directories(SYSTEM
  ${OPENGL_INCLUDE_DIR}
  ${PYTHON_INCLUDE_PATH}
 )
-include_directories(src ${catkin_INCLUDE_DIRS})
+include_directories(src SYSTEM ${catkin_INCLUDE_DIRS})


I don't want to use SYSTEM, it's known to cause all sorts of problems with the include order on some OS's.

Please use the diagnostics pragma's to disable the particular warnings when including the problem headers. See:

https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html

Here's an example of it in use:

https://github.com/ros2/rmw_connext/blob/master/rmw_connext_cpp/src/functions.cpp#L26-L40

Could you elaborate on that? I have never had or heard of problems with marking include directories as system directories and it is the cleanest way for the compiler to ignore errors in external headers.

It is the least compiler specific and it does not depend on the exact contents of the external headers, which is not under control of this package (so can change and fix or add new warnings).

Sure, the SYSTEM directive ends up using -isystem as the option to the compiler. The -isystem paths are always looked at after -I, and if a path is specified with both, the -I entry is ignored:

From https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html:

All directories named by -isystem are searched after all directories named by -I, no matter what their order was on the command line. If the same directory is named by both -I and -isystem, the -I option is ignored.

So that means that when you use it with header paths that may be coming from a workspace, like ~/my_ws/install/include, and from somewhere else you have an -I/opt/ros/kinetic/include, then you can get in a situation where you use the headers from /opt/ros rather than the headers from ~/my_ws/install even when the workspace should take precedence. Also catkin_INCLUDE_DIRS is sorted by workspace prefix to avoid a similar problem, so adding these include unevenly may result in unexpected, and hard to debug, behavior when using a workspace.

Also, on OS X, the isystem flag is used to control which SDK you're using and it caused problems in the past to have more instances of that, though I think CMake/Apple fixed that at some point.

I think in this case it would be ok to use it since we're putting all headers under SYSTEM, but because of the subtle issues with isystem I don't want to introduce it in case someone adds more headers later without understanding the pitfall. If you don't want to take the time to suppress the warns with the #pragma's, then I'll take care of it. Thanks for the fixes you've provided so far!

I see, I did not take into account multiple workspaces. That issue is unfortunate, as it doesn't seem like something that can easily be worked around.

My main hesitation with using pragmas is that they should be maintained properly to stay in sync when new warnings are added by the external dependency. Also, removing unneeded pragmas becomes difficult since it would mean reintroducing warnings when compiling with older versions of the external dependency. On the other hand -isystem more clearly communicates the intent ("these headers are not mine, don't blame me") and doesn't require maintenance.

Is there perhaps a pragma for gcc and clang which has the same effect on an included header as -isystem? That would fix the problem without adding a maintenance burden while avoiding include path order issues.

Hmm, there is this:

#pragma GCC system_header This pragma takes no arguments. It causes the rest of the code in the current file to be treated as if it came from a system header. See System Headers.

But the rest of the file is too broad. If that could be turned off again it would be perfect...

Depending on the exact behaviour of that pragma it could be possible to wrap offending headers in a header with that pragma, but that is getting a bit insane...

Yeah, I was thinking that too. On the one hand it seems a bit overkill, but on the other hand a separate wrapper header would be the best way anyway if the offending include is used multiple times. And then #pragma GCC system_header is viable option again.

I'll go for that I suppose =]

There is still a larger maintenance burden of course, since external dependencies can still introduce warnings in other headers. I'm now wondering again if including everything external with SYSTEM isn't actually the best option.

An additional complication is that I don't get any warnings from external dependencies locally, even without the CMakeLists.txt modifications and a clean configure+build (gcc 6 defaults to c++11, where this specific warning is gone). I can go by the log from Jenkins of course, but it does illustrate the problem with manually disabling warnings from specific includes.

Well, as I said, I'm not interested in maintaining the use of SYSTEM, but I'll take on the work of maintaining suppression of warnings from other headers. I can't take care of this today, but perhaps this weekend I can add what's needed to your PR here and merge it.

Fair enough. I'll remove the CMakeLists.txt changes commit.

de-vri-es · 2016-06-10T21:15:36Z

Managed to trigger the warnings locally by adding compiling with -std=c++03. I had to add a wrapper header since there does not appear to be a specific pragma to disable this warning. If there is it is not shown by gcc:

warning: defaulted and deleted functions only available with -std=c++11 or -std=gnu++11

It is also a rather serious warning. Technically resource_retriever is now forcing everything depending on it to compile with c++11 support. I'm not against that, but it is something to think about. I'll take that discussion to resource_retriever though.

wjwwood · 2016-06-10T21:18:12Z

Yes, they should probably export the flag in that case. As of Kinetic they're allowed to use C++11 if they want.

de-vri-es · 2016-06-10T21:43:53Z

If interested, see ros/resource_retriever#7 for that discussion.

de-vri-es · 2016-06-10T21:45:48Z

Oh, and of course another option to get rid of the resource_retriever warning is to compile with C++11 support. Then the crufty wrapper header can be removed, and even better, we get C++11 :)

wjwwood · 2016-06-16T20:44:14Z

Ok, I undid the fake system header commit and turned on c++11 (faf97be) and then merged to master.

wjwwood · 2016-06-16T20:44:45Z

Thanks for the contribution and your patience.

de-vri-es · 2016-06-16T20:49:54Z

You too ;)

And great to have C++11 in rviz

…/pr-1018 Don't pass screw_display.hpp to the moc generator. (backport ros-visualization#1018)

de-vri-es added 2 commits June 3, 2016 10:45

Fix unused parameter warnings.

d28b37c

Swap out deprecated Ogre::MovableObject::setUserAny.

41c2a66

wjwwood reviewed Jun 10, 2016
View reviewed changes

de-vri-es force-pushed the fix-warnings branch from b2ad35f to 41c2a66 Compare June 10, 2016 20:32

Silence warnings from resource_retriever.

dd1f441

wjwwood merged commit dd1f441 into ros-visualization:kinetic-devel Jun 16, 2016

130s pushed a commit to 130s/rviz that referenced this pull request Aug 21, 2024

Merge pull request ros-visualization#1019 from ros2/mergify/bp/humble…

edda46c

…/pr-1018 Don't pass screw_display.hpp to the moc generator. (backport ros-visualization#1018)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compilation warnings #1019

Fix compilation warnings #1019

de-vri-es commented Jun 3, 2016

de-vri-es commented Jun 8, 2016

wjwwood Jun 10, 2016

de-vri-es Jun 10, 2016 •

edited

Loading

wjwwood Jun 10, 2016 •

edited

Loading

de-vri-es Jun 10, 2016

de-vri-es Jun 10, 2016 •

edited

Loading

de-vri-es Jun 10, 2016

de-vri-es Jun 10, 2016 •

edited

Loading

wjwwood Jun 10, 2016

de-vri-es Jun 10, 2016

wjwwood Jun 10, 2016

de-vri-es commented Jun 10, 2016 •

edited

Loading

wjwwood commented Jun 10, 2016

de-vri-es commented Jun 10, 2016

de-vri-es commented Jun 10, 2016

wjwwood commented Jun 16, 2016

wjwwood commented Jun 16, 2016

de-vri-es commented Jun 16, 2016

Fix compilation warnings #1019

Fix compilation warnings #1019

Conversation

de-vri-es commented Jun 3, 2016

de-vri-es commented Jun 8, 2016

wjwwood Jun 10, 2016

Choose a reason for hiding this comment

de-vri-es Jun 10, 2016 • edited Loading

Choose a reason for hiding this comment

wjwwood Jun 10, 2016 • edited Loading

Choose a reason for hiding this comment

de-vri-es Jun 10, 2016

Choose a reason for hiding this comment

de-vri-es Jun 10, 2016 • edited Loading

Choose a reason for hiding this comment

de-vri-es Jun 10, 2016

Choose a reason for hiding this comment

de-vri-es Jun 10, 2016 • edited Loading

Choose a reason for hiding this comment

wjwwood Jun 10, 2016

Choose a reason for hiding this comment

de-vri-es Jun 10, 2016

Choose a reason for hiding this comment

wjwwood Jun 10, 2016

Choose a reason for hiding this comment

de-vri-es commented Jun 10, 2016 • edited Loading

wjwwood commented Jun 10, 2016

de-vri-es commented Jun 10, 2016

de-vri-es commented Jun 10, 2016

wjwwood commented Jun 16, 2016

wjwwood commented Jun 16, 2016

de-vri-es commented Jun 16, 2016

de-vri-es Jun 10, 2016 •

edited

Loading

wjwwood Jun 10, 2016 •

edited

Loading

de-vri-es Jun 10, 2016 •

edited

Loading

de-vri-es Jun 10, 2016 •

edited

Loading

de-vri-es commented Jun 10, 2016 •

edited

Loading