Skip to content
This repository has been archived by the owner on Aug 3, 2020. It is now read-only.

fix missed mandatory -std=c++11 flag #181

Merged
merged 1 commit into from Feb 3, 2017
Merged

fix missed mandatory -std=c++11 flag #181

merged 1 commit into from Feb 3, 2017

Conversation

4eetah
Copy link
Contributor

@4eetah 4eetah commented Jan 26, 2017

collada_parser,kdl_parser,urdf: add c++11 flag,
collada_parser: replace typeof with ansi typeof
builded/tested on gentoo

@4eetah
Copy link
Contributor Author

4eetah commented Jan 26, 2017

Can't actually reproduce the build failure on gentoo, build and tests compiled smoothly

@wjwwood
Copy link
Member

wjwwood commented Jan 26, 2017

Well, we'll have a look at it asap, but if you have time to look at the error and reason about it then that would be good. I can't merge the patch if it doesn't build on our main target platform (Ubuntu Xenial for Kinetic).

@4eetah
Copy link
Contributor Author

4eetah commented Jan 26, 2017

I actually looked at it, that's why I mentioned I can't reproduce the issue on my machine. Seems like the modules build is ok, the tests build fails though, can't really think of how this patch might affect the tests.

@wjwwood
Copy link
Member

wjwwood commented Jan 27, 2017

add_compile_options applies to the whole folder, so it would also affect the tests. It is likely that something about the combination of the explicit -std=c++11 combined with the version of gcc on Xenial has broken something in the tests. As I said, we'll get to it asap, but it probably won't be for a while.

@4eetah
Copy link
Contributor Author

4eetah commented Jan 27, 2017

👍 there are two of them std::isnan and boost::math::isnan
so after including c++11 isnan seems like an issue
for some reason they don't interfere on my machine =)

@wjwwood
Copy link
Member

wjwwood commented Jan 27, 2017

Thanks! @clalancette or @sloretz can one of you review this after the build farm comes back with a result and then merge if it looks good to you?

@wjwwood
Copy link
Member

wjwwood commented Jan 27, 2017

Unfortunately we now have a compiler warning which is new with C++11 😝. I think the right thing to do is to disable this warning when importing from collada (source of the issue) using pragma's, see:

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

Perhaps @clalancette or @sloretz could help with that, unless @Den4ix feels comfortable doing it.

collada_parser,kdl_parser,urdf: add c++11 flag,
collada_parser: replace typeof with ansi __typeof__
builded/tested on gentoo
@4eetah
Copy link
Contributor Author

4eetah commented Jan 27, 2017

this becoming kinda funny, I hope it won't produce another warn/err =)

@wjwwood
Copy link
Member

wjwwood commented Jan 27, 2017

Awesome, build farm is happy now, thanks!

@wjwwood wjwwood requested a review from sloretz January 27, 2017 01:50
@wjwwood wjwwood added the bug label Jan 27, 2017
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

Looks good to me

@sloretz
Copy link
Contributor

sloretz commented Jan 30, 2017

@wjwwood I noticed a similar pull request #145 tries to be windows compatible. Is that a standard pull requests should be held to going forwards?

@wjwwood
Copy link
Member

wjwwood commented Jan 30, 2017

Windows support is "nice to have", but I don't ask others to do in pr's because we have no CI that tests it on Windows for them, and I don't expect them to be able to do so. One of us could extend this pr to support Windows if we had the interest and time to do so, but I don't right now.

@sloretz sloretz merged commit 3c7933b into ros:kinetic-devel Feb 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants