Compile with warnings and fix them

[VictorLamoine]

• Compile with -Wall and -Wextra flags
• Fixes compilation warning

This warning comes from MoveIt, I'll make a pull request to fix this issue, this bloats compilation logs when using warnings:

In file included from /home/victor/code/catkin_workspace/src/descartes/descartes_core/include/descartes_core/robot_model.h:23:0,
                 from /home/victor/code/catkin_workspace/src/descartes/descartes_core/include/descartes_core/trajectory_pt.h:33,
                 from /home/victor/code/catkin_workspace/src/descartes/descartes_trajectory/include/descartes_trajectory/trajectory.h:29,
                 from /home/victor/code/catkin_workspace/src/descartes/descartes_trajectory/src/trajectory.cpp:25:
/opt/ros/kinetic/include/moveit/kinematic_constraints/kinematic_constraint.h: In member function ‘virtual void kinematic_constraints::KinematicConstraint::print(std::ostream&) const’:
/opt/ros/kinetic/include/moveit/kinematic_constraints/kinematic_constraint.h:148:47: warning: unused parameter ‘out’ [-Wunused-parameter]
   virtual void print(std::ostream& out = std::cout) const
                                               ^~~~

Main issues fixed:

• Comparison between unsigned / signed integers, using std::size_t solves them all
• Removed un-used parameters in functions (I think some of them could be deleted but I don't know the code well enought to be sure it can)

To be done before merging:

• Testing (someone ought to test this, other than me)
• Wait for a PR to be merged in MoveIt to remove the warning

[VictorLamoine]

Pull request to remove the warning from MoveIt is here: https://github.com/ros-planning/moveit/pull/849

[Jmeyer1292]

@VictorLamoine I'll go through these and test/merge after work today.

[Jmeyer1292]

Additionally, @VictorLamoine I got the sense of what you're trying to do from the Discourse page. Can you share URDF models & perhaps tool paths with me, either in public or privately via email? I would like to help where I can get you setup on the right foot.

[VictorLamoine]

I'll make a public repository with a simple setup but it's going to take some time as I have plenty to do :) I will ping you when it is ready!

[VictorLamoine]

My MoveIt merge request has been merged. Tell me if you want me to remove the -Wall -Wextra compile flags on the packages.

[Jmeyer1292]

I'm okay with all the changes except maybe the ones in SparsePlanner. I always welcome stuff being built with all possible, relevant warnings.

[VictorLamoine]

I rebased against current master and I fixed the problem with findNearestSparsePointIndex and getDensePointIndex

[Jmeyer1292]

@VictorLamoine I'm in an uncomfortable situation because I don't have any real tests with respect to the code in sparse planner specifically and I'm not sure if it works now much less after the changes you made. The original author does not have time to look at it. So:

1. If you've studied this and feel confident that the code is logically correct, then I will merge it.
2. If not, I would like to either revert the code in sparse_planner.cpp alone and suppress warnings about uint/int comparisons, OR...
3. Cast the unsigned types (such as calls to size() to int, instead of moving ints to unsigned).

I hate to be annoying, but this is my request to you on this one. Given this codes status, I'm inclined to mark it deprecated and remove it in the future unless there is someone who is willing to take on the burden on maintenance.

[jdlangs]

Just wanted to also chime in to mention my impression is that current C++ wisdom seems to be to move away from unsigned types. E.g., the google style guide:

You should not use the unsigned integer types such as uint32_t, unless there is a valid reason such as representing a bit pattern rather than a number, or you need defined overflow modulo 2^N. In particular, do not use unsigned types to say a number will never be negative. Instead, use assertions for this.

and

The best advice we can provide: try to use iterators and containers rather than pointers and sizes, try not to mix signedness, and try to avoid unsigned types (except for representing bitfields or modular arithmetic). Do not use an unsigned type merely to assert that a variable is non-negative.

So it's probably best to not let unsignedness spread through the program, even if it happens to be needed locally in a for loop.

[VictorLamoine]

I'm not sure Google style guide is a reference...! The C++11 standards states that an array or std container index must be a std::size_t, it is the only safe and portable way to index arrays. Anything else is either unsafe, not portable or both.

@Jmeyer1292 that is exactly where automated tests would have been very helpful :smiley: I'll revert the changes in the sparse_planner and create an other WIP pull request with the very same modifications.

[VictorLamoine]

PR updated, it fixes the warnings for everything but descartes_planner package, I disabled the warnings in this package because they are not fixed.

@Jmeyer1292 do not hesitate to ask me to do some modifications if there is anything you don't like.

[Jmeyer1292]

@VictorLamoine Thanks for your patience with us.