Fixes for compiler warnings

yvaind commented 5 years ago

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.

ethzasl-jenkins commented 5 years ago

Can one of the admins verify this patch?

yvaind commented 5 years ago

@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...

yvaind commented 5 years ago

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.

ethz-asl / curves

Fixes for compiler warnings #77