posemath: drop the dead knobs and give it its own directory - #4387
posemath: drop the dead knobs and give it its own directory#4387grandixximo wants to merge 2 commits into
Conversation
posemath.h carries four switches that have only ever had one setting, and
they hide what the declarations actually say.
USE_CONST, USE_CCONST and USE_REF are defined unconditionally a few lines
above the blocks that test them, so PM_CONST and PM_CCONST are always
const and PM_REF is always a reference. Spelling them out turns
declarations like
PM_CARTESIAN(PM_CONST PM_SPHERICAL PM_REF s);
into what they have always meant:
PM_CARTESIAN(const PM_SPHERICAL & s);
INCLUDE_POSEMATH_COPY_CONSTRUCTORS is guarded by __cplusplus < 201103L, so
it is never defined in a tree built as C++20. The comment beside it
already explains that the compiler generates better copy constructors than
the ones it hides, and every one of them has been dead code since the move
to C++11.
PM_LOOSE_NAMESPACE would typedef VECTOR, MATRIX, POSE and friends into the
global namespace. Nothing defines it.
The remaining #if 0 blocks go too: the norm() declarations and their
definitions, and a note about a call being ambiguous on g++ 2.8 and 2.9.
The deliberate poison macro for pmCartNorm() stays, since it is there to
turn a use into a compile error rather than to be compiled out.
No behaviour change: the preprocessor already resolved all of this the same
way on every build.
posemath is not part of NML and never has been. It includes nothing from libnml, uses none of its types, and builds into its own shared library, libposemath.so. The only edge between them runs the other way: cms_pm.cc includes <posemath.h> to serialise the classes. Sitting inside libnml/ made it look like an NML component, which is misleading for anyone reading the tree and awkward for anything that wants to depend on the maths without the messaging. Pure move. The files are unchanged, and what follows is the paths that named the old location: the Submakefile's own prefixes, the SRCHEADERS entries and per-module object lists in src/Makefile, the two USE_TOPDIR includes in tpcomp.comp, and the copyright stanza in debian/. SUBDIRS gains libposemath as an entry of its own rather than one buried in the libnml group, since that grouping was the thing being corrected.
| /* some nice constants */ | ||
|
|
||
| #define PM_PI 3.14159265358979323846 | ||
| #define PM_PI_2 1.57079632679489661923 | ||
| #define PM_PI_4 0.78539816339744830962 | ||
| #define PM_2_PI 6.28318530717958647692 | ||
|
|
||
| #ifdef PM_LOOSE_NAMESPACE |
There was a problem hiding this comment.
We may actually test if these should be redefined as:
#define PM_PI M_PI
...There was a problem hiding this comment.
Tested:
PM_PI == M_PI bit identical
PM_PI_2 == M_PI_2 bit identical
PM_PI_4 == M_PI_4 bit identical
PM_2_PI != M_2_PI 6.2831853071795862 vs 0.63661977236758138
PM_2_PI == 2*M_PI bit identical
Three map straight across. The fourth is a trap: M_2_PI in libm is 2/pi, not 2pi, so the obvious substitution is wrong by a factor of about 9.87 everywhere PM_2_PI is used; it wants 2M_PI written out.
Since the values are identical today nothing is waiting on this. I would do it with the rest of the constants once the split moves them into their own header.
There was a problem hiding this comment.
The "correct" values are in the system/libc defines. Using our own values is an ancient compatibility thing.
However, the RTAI build uses a kernel supplied header and that may not have these defines. In rtapi/rtapi_math.h we can also see the value of pi being defined as the long double value (digits get truncated). Maybe we need to check the other values in rtapi_math.h too and define if not present.
The PM_2_PI vs. M_2_PI is interesting. At least that value has a real reason to be here.
There was a problem hiding this comment.
The kernel branch includes no math.h and fills in only M_PI, M_PIl and M_PI_2l, so M_PI_2, M_PI_4 and the other ten are absent there, and the truncated digits turn out to be cosmetic since every fallback is already glibc's value.
Sent as #4395, which defines the missing ones under the same guard.
2 participants
Following on from the review discussion in #4375, where the posemath headers came up and the answer was that the code has been misplaced in libnml for some time. This is the first two steps of the five I sketched there, and the two that stand on their own.
A. Drop the knobs that are never turned.
USE_CONST,USE_CCONSTandUSE_REFare defined unconditionally at the top of the header and selectPM_CONST,PM_CCONSTandPM_REF. Every declaration in the file is written through that indirection, so reading a signature means resolving three macros to find out that they meanconst,constand&. The macros are substituted out.INCLUDE_POSEMATH_COPY_CONSTRUCTORSis guarded by#if __cplusplus < 201103L, and the tree builds as C++20, so its 13 blocks in each file have not been compiled in a long time.PM_LOOSE_NAMESPACEis defined nowhere. Four#if 0blocks go with them.What stays is the one deliberate poison macro for
pmCartNorm(), which exists to make a use fail to compile rather than to be compiled out.No behaviour change: the preprocessor already resolved every one of these the same way on every build in the tree. 338 lines out, 50 in.
B. Move it out of libnml.
git mv src/libnml/posemath src/libposemath. posemath includes nothing from NML and uses nothing from it; the only edge between them runs the other way, incms_pm.cc. The move is the sources plus the paths that named the old location: the Submakefile prefixes,SRCHEADERSand the per-module object lists insrc/Makefile, twoUSE_TOPDIRincludes intpcomp.comp, and thedebian/copyrightstanza.libposemathgets its ownSUBDIRSentry rather than one buried in the libnml group. All eight source files are detected as pure renames.Testing.
Configured
--with-realtime=uspaceand built clean, no errors and no warnings.libposemath.so.0exports the same 390 symbols before and after, byte identical lists fromnm -D --defined-only. That is the evidence that A changes nothing: it is a large diff that the compiler cannot tell apart from the original.include/holds the same 38 headers with the same names, so the installed surface is untouched by the move.scripts/runtestsover blendmath, realtime-math, interp, ccomp and matrixkins: 90 run, 90 successful, 0 failed, 1 skipped, 0 shmem errors.All nine kinematics modules that link a posemath object load under
halrun: genser, scara, puma, genhex, penta, 5axis, three21, rotarydelta, triv.What comes next, separately.
C splits the header by language behind the existing umbrella, D folds
emcpos.handemcpose.hinto one header beside posemath, and E takesgomath.h,gotypes.handsincos.hoff the exported list. Those are written and tested but want their own review, since each changes what the tree installs.