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

Fixes for compiler warnings #77

Merged
merged 6 commits into from
Apr 2, 2019
Merged

Fixes for compiler warnings #77

merged 6 commits into from
Apr 2, 2019

Conversation

yvaind
Copy link
Contributor

@yvaind yvaind commented Mar 29, 2019

Fixed some warnings that would be triggered in downstream packages including header files from this repo when building with the '-Wall -Wextra -Wpedantic' compiler flags.

…cluding header files from this repo when building with the '-Wall -Wextra -Wpedantic' compiler flags.
@ethzasl-jenkins
Copy link

Can one of the admins verify this patch?

@yvaind yvaind requested a review from harmishhk March 29, 2019 18:05
@yvaind yvaind requested a review from remod March 29, 2019 21:23
@yvaind
Copy link
Contributor Author

yvaind commented Mar 29, 2019

@goodfaiter I'm sorry, but you need to unapprove and review again. I added about twice as many new changes and we need to discuss a few things. Since these may be relevant for deciding what to do with other open-sourced packages, I also added you @remod as a reviewer.

  • There are at least 3 different ways errors are logged/handled: std::cerr, std::runtime_error, and CHECK (a google macro defined in logging.h). I think we should adhere to one standard, the ideal one maybe being MELO. However, this would introduce another dependency.
  • There are tons of virtual and overriden functions that are not implemented and will not even trigger a runtime error if used. There are a few places where an error is printed, but not in a consistent manner (see previous item). IMO all of them should print an error and we need to decide which way to go.
  • The override specifier is not used a single time, although plenty of virtual functions get overriden.
  • Adding clang tooling would be nice. But it also means an additional, new dependency.
  • There are files that are not compiled when building curves, but only when building curves_ros. I fear that there may be even more files inside curves that only get compiled when used by another package.
  • There are certainly more issues...

curves/CMakeLists.txt Outdated Show resolved Hide resolved
@yvaind
Copy link
Contributor Author

yvaind commented Apr 1, 2019

I discussed with @remod and as you suggested @goodfaiter, we think it's best to address above issues (#81 , #80 , #79 , #78 ) in separate PRs.

@yvaind yvaind merged commit 6af4419 into master Apr 2, 2019
@yvaind yvaind deleted the fix/compiler_warnings branch April 2, 2019 07:21
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.

5 participants