Re: [eigen] Mapping array of scalars into quaternions

[Benoit Jacob <jacob.benoit.1@xxxxxxxxx>]

Hi,

Thanks for your patch.

It is mostly ready to be pushed, but I have one potentially serious
comment. It is basically adding EIGEN_STRONG_INLINE everywhere. While
this is pretty harmless (and often useful) on trivial functions, this
patch also adds EIGEN_STRONG_INLINE on large functions such as:

  EIGEN_STRONG_INLINE static Quaternion<Scalar> run(...){
    return Quaternion<Scalar>
    (
      a.w() * b.w() - a.x() * b.x() - a.y() * b.y() - a.z() * b.z(),
      a.w() * b.x() + a.x() * b.w() + a.y() * b.z() - a.z() * b.y(),
      a.w() * b.y() + a.y() * b.w() + a.z() * b.x() - a.x() * b.z(),
      a.w() * b.z() + a.z() * b.w() + a.x() * b.y() - a.y() * b.x()
    );
  }

Everytime you add a EIGEN_STRONG_INLINE on a function that has a
nontrivial body of its own (rather than just forwarding the call to
another function), I need to see some justification (as far as I can
see, that would have to be justified by performance measurements). I'm
by no means an expert, i'm really honestly asking. My fear is about
uselessly increasing code size (and killing instruction caches).

Another such nontrivial function is _transformVector.

I also have a few superficial comments:

First, "inline" keywords need only be put on function definitions, no
need to put them on function definitions [1]. I realize that
Quaternion.h already had this problem before your patch. I just am
noticing it now, because EIGEN_STRONG_INLINE takes more space on my
screen.

Second, in the unit test (thanks for writing it), you could just
reserve static arrays with EIGEN_ALIGN16, see what we do in
MatrixStorage.h.

Third, when you significantly extend/rewrite a file, add yourself a
copyright line. I already added you one on Quaternion.h, also do it on
quaternion.cpp.

Thanks,
Benoit

[1] http://www.parashift.com/c++-faq-lite/inline-functions.html#faq-9.9

2009/11/12 Mathieu Gautier <mathieu.gautier@xxxxxx>:
> Hi,
>
>> Also, it would help to read some rationalizations:
>> * why the EIGEN_STRONG_INLINE in front of cross product? Did you
>> benchmark an improvement here?
>
> I remove this part, I have to inspect this more closely.
>
> I update the documentation, and add the test. I also had to modify the
> EIGEN_INHERIT_ASSIGNEMENT_OPERATORS macro to use only operator= and to
> handle icc with visual studio.
>
> The changeset :
> * add Map<Quaternion> test based on Map from test/map.cpp
> * replace implicit constructor AngleAxis(QuaternionBase&) by an explicit
> one, it seems ambiguous for the compiler
> * remove explicit constructor with conversion type quaternion(Quaternion&):
> conflict between constructor.
> * modify EIGEN_INHERIT_ASSIGNEMENT_OPERATORS to suit Quaternion class
>
> --
> Mathieu Gautier
>