Splitted base/types to seperate cpp and hpp files

[hemker]

We splitted the base-types header-only lib into a separate .cpp/.hpp implementation due to very long orogen parsing times. The old implementation needed about 8 minutes (on a SSD) to finish the orogen phase in base/orogen/types. With the new concept, an enhancement of nearly 6 minutes (parsing times around 2 minutes) can be achieved. Furthermore, we removed some deprecated stuff.

Split Pose.hpp into implementation and header

Float.hpp: Use std function, not boost ones

Eigen: remove isnotnan and isfinite

This is a duplicate of the buildin
allFinite()

move guaranteeSPD into TwistWithCovariance.

This one includes SVD, which is a big include in the base eigen.

moved Wrench.hpp to new src

moved TwistWithCovariance.hpp to new lib/src

moved Waypoint to new src folder

re-patched new isnan interface

splitted TransformWithCovariance

splitted Timueout

moved headers to cpp files

splitted TimeMark

removed inlining

splitted Time

splitted Temperature

moved Point

splitted Pressure

fixed compile errors

Added correct PC file for base-types, made base-lib a dummy

moved CircularBuffer

moved Deprecated

moved JointLimitRange

moved JointLimits

moved JointState

moved JointTransform

moved JointsTrajectory

moved NamedVector

moved Odometry

added odometry

ported rest of types

re-removed vector included

changed dep to right lib name

compile fixes

[jakobs]

Would be great if a few people could test this, since this is a fairly big change.

[jmachowinski]

Tested, works.
Some packages reported now missing headers, but this is actually a positive side effect.

[jakobs]

One Problem I have with this PR, is that it is a single commit. It would be much better to have multiple commits, since there seem to be different changes involved. Also, (I havent' checked) it would be important imho to have the original authorships preserved.

[chhtz]

Just glancing over it, I'd also prefer having it split into smaller commits.

At least one flaw: You copied the old/broken guaranteeSPD() which I recently fixed (#93)

About std::isfinite, etc: Is rock-master officially >=C++11-only? (I would not object, just wanted to ask if that is official policy).

In the cpp-files, you could embrace everything into namespace base { ... } instead of prefixing everything with base::

Also, I'd prefer keeping the very simple getter/setter functions inlined in the header (I did not benchmark that, but I'd guess some will have measurable impact on runtime).

[hemker]

@jakobs I made the tests compile again. But there's one inconvenience: Since we moved the method guaranteeSPD from base/Eigen.hpp to TwistWithCovariance.cpp - because it's the only class where it is used - the test suite test_Eigen.cpp cannot compile anymore. What to do now? Remove test_Eigen.cpp? Or make the guaranteeSPD an interface function of TwistWithCovariance. But that would cause to put the templated method into the header again and make it public.

[jmachowinski]

It's a bit hacky, but include the cpp file in the test. That should work fine.

[hemker]

Done. Works fine

[chhtz]

This of course makes the declaration of guaranteeSPD in base/Eigen.hpp essentially useless.
I'm not totally convinced that this really is a useful function -- considering that "everything worked fine"™ for very long with a broken implementation of that function.
@jhidalgocarrio (or anyone else): Did using this function make a difference somewhere?

[jhidalgocarrio]

Please, do not move the guaranteeSPD method to TwistWithCovariance.cpp
As @hemker suggested, make the guaranteeSPD an interface function of TwistWithCovariance and put the guaranteeSPD templated method into the header again and make it public.
The idea is to use the guaranteeSPD method for any arbitrary Eigen matrix.

[chhtz]

We could leave the function in Eigen.hpp but hide the actual implementation in a function which accepts only const Matrix<S,Dynamic,Dynamic>&, implement that function in a cpp file and instantiate that only for double, float, complex<double> and complex<float>.

Addendum: I do think that removing #include<Eigen/Eigenvalues> from the base types headers makes a lot of sense (that header is quite heavy and barely used elsewhere).

[jmachowinski]

We could also leave it a template and move it into some other header. It was moved out of the Eigen.hpp header, as it adds a couple of includes.

[jhidalgocarrio]

Both solution could work, but please do not move it to TwistWithcCovariance.
I guess @jmachowinski prefers guaranteeSPD out of Eigen.hpp for compilation time, but moving it to another header does not really solve the compilation time problem. It moves it to another header which needs to be included when using the method.
what @chhtz proposes is more reasonable to me +1. Meaning that this PR is all about "very long parsing and compilation time".

[chhtz]

Moving it to a separate header (which is not included by Eigen.hpp) would make a difference, because a lot of packages are (indirectly) including Eigen.hpp (but don't need guaranteeSPD). It would require users of that function to add an #include, of course.

[jhidalgocarrio]

Alright. I am also fine with it and it seems the easiest 👍

[hemker]

I moved guaranteeSPD to it's own header, removed Eigen includes from TwistWithCovariance and made its .cpp dependend on new Matrix.hpp

[chhtz]

There was still the concern #94 (comment) to keep the very trivial functions inside the header, i.e., at least the one-liner functions which are unlikely to change (and don't depend on including heavy external headers). This was also addressed on the mailing list.

Other than that, I don't have objections, though I did not actually test the most recent version.

Regarding orogen/compile time: This change will actually save a lot of additional time since at the moment (almost) every package needs to be rebuild even for small changes in function bodies.

[jmachowinski]

There was still the concern #94 (comment) to keep the very trivial functions inside the header, i.e., at least the one-liner functions which are unlikely to change (and don't depend on including heavy external headers). This was also addressed on the mailing list.

This is not critical and can be changed over time if needed. I would prefer to merge now, so that we don't collide any more with other merges going into base types.

[jakobs]

I believe this is not correct (and the compiler warns about it!). It was static inline before, which is fine since the header was always in the respective compilation unit. When declaring it, static means the function is local to this compilation unit, and will not get exported.

[chhtz]

I think this should be declared as friend method inside the class. Alternatively, (also inside the class) as

Temperatur operator+(const Temperature & other) const;

[jakobs]

just removing the static should be ok. It was working with the original code.

[chhtz]

@jmachowinski All right, I'm fine with that (but I guess, I don't have the final call on this).

[doudou]

All right, I'm fine with that (but I guess, I don't have the final call on this).

This is base-types. No one does have "the final call on this"

[doudou]

And 👍 on Janosch's point. It's good enough for now, let's dwelve into C++11 / optimizations later.

[jakobs]

Ok, I don't have any more objections. From my point this is good to merge.

[chhtz]

I guess this static must be removed as well (this gives a [-Wunused-function] warning, too).

[doudou]

Especially because a static of a declaration looks absolutely wrong.... You can't be static without a body defined somewhere.

[jmachowinski]

Compiles fine in fresh bootstrap and my own project