roboticslab-uc3m / kinematics-dynamics

Kinematics and dynamics solvers and controllers.
https://robots.uc3m.es/kinematics-dynamics/
GNU Lesser General Public License v2.1
19 stars 12 forks source link

Locally (re)defined VOCABs are no longer compatible with YARP devel #158

Closed PeterBowman closed 6 years ago

PeterBowman commented 6 years ago

Offending code:

https://github.com/roboticslab-uc3m/kinematics-dynamics/blob/9e87f28b187aa7d384698cb4dc6f50283789ece1/libraries/YarpPlugins/ICartesianSolver.h#L9

https://github.com/roboticslab-uc3m/kinematics-dynamics/blob/9e87f28b187aa7d384698cb4dc6f50283789ece1/libraries/YarpPlugins/ICartesianControl.h#L12

From Travis:

[ 17%] Building CXX object libraries/YarpPlugins/AmorCartesianControl/CMakeFiles/AmorCartesianControl.dir/ICartesianControlImpl.cpp.o
In file included from /home/travis/build/roboticslab-uc3m/kinematics-dynamics/libraries/YarpPlugins/AmorCartesianControl/AmorCartesianControl.hpp:12:0,
                 from /home/travis/build/roboticslab-uc3m/kinematics-dynamics/libraries/YarpPlugins/AmorCartesianControl/ICartesianControlImpl.cpp:3:
/home/travis/build/roboticslab-uc3m/kinematics-dynamics/libraries/YarpPlugins/ICartesianControl.h:12:28: error: expected unqualified-id before ‘int’
 #define VOCAB(a,b,c,d) ((((int)(d))<<24)+(((int)(c))<<16)+(((int)(b))<<8)+((int)(a)))
                            ^
/home/travis/build/roboticslab-uc3m/kinematics-dynamics/libraries/YarpPlugins/ICartesianControl.h:12:28: error: expected ‘)’ before ‘int’
/home/travis/build/roboticslab-uc3m/kinematics-dynamics/libraries/YarpPlugins/ICartesianControl.h:12:28: error: expected ‘)’ before ‘int’
/home/travis/build/roboticslab-uc3m/kinematics-dynamics/libraries/YarpPlugins/ICartesianControl.h:12:28: error: expected ‘)’ before ‘int’
/home/travis/build/roboticslab-uc3m/kinematics-dynamics/libraries/YarpPlugins/ICartesianControl.h:12:28: error: expected ‘)’ before ‘int’
make[2]: *** [libraries/YarpPlugins/AmorCartesianControl/CMakeFiles/AmorCartesianControl.dir/ICartesianControlImpl.cpp.o] Error 1
make[1]: *** [libraries/YarpPlugins/AmorCartesianControl/CMakeFiles/AmorCartesianControl.dir/all] Error 2
make: *** [all] Error 2

Note this does not happen in a similar repository, asrob-uc3m/yarp-devices (which also redefines the VOCAB macro, ref). Investigate this at asrob-uc3m/robotDevastation, too (ref).

I suspect https://github.com/robotology/yarp/pull/1764 of being the root cause.

PeterBowman commented 6 years ago

Macros are weird. For instance, you can define macro A on which macro B depends after the definition of B itself, as long as A is defined before it is used in normal C/C++ code (ref). This kinda defeats my original idea of placing a #undef VOCAB before exiting ICartesianControl.h and ICartesianSolver.h (both redefine VOCAB), but it would lead to errors anyway (undeclared VOCAB, requested by e.g. VOCAB_CC_NOT_CONTROLLING and the like).

I'm eager to adopt C++11's constexpr in our code, but how to avoid multiple redefinition errors? Also, should I drop all macros altogether, that is, not just the VOCAB macro function, but also VOCAB_CC_MOVJ and others?

PeterBowman commented 6 years ago

Alternatively, if sticking to old C macros: rename VOCAB() to RL_VOCAB().

PeterBowman commented 6 years ago

Conclusion: preprocessor macro names should be unique, but this has been violated (due to our misuse) after constexpr functions were introduced upstream. Adopting constexprs is not straightforward because of redefinitions. I'd definitely lean towards the RL_VOCAB macro name solution.

PeterBowman commented 6 years ago

As suggested by @jgvictores, ROBOTICSLAB_VOCAB looks better (in my early days, I myself was confused about the rl prefix used throughout the org).