Re: [eigen] Mapping array of scalars into quaternions |

[ Thread Index | Date Index | More lists.tuxfamily.org/eigen Archives ]

*To*: eigen@xxxxxxxxxxxxxxxxxxx*Subject*: Re: [eigen] Mapping array of scalars into quaternions*From*: Benoit Jacob <jacob.benoit.1@xxxxxxxxx>*Date*: Thu, 12 Nov 2009 19:42:32 -0500*Dkim-signature*: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:content-type; bh=Ag48Js9iJKoELpzwZg+VNEST/OfwTafA5J4hAxR20Vw=; b=lZG2JZ6aMctm5wRTJb/5bjXiB8vYZQBAfBvA09EGF//zpIyQ/dC59bcZyZVUwS8EuY 4scGPxcGMNABWZ7zkjBASgzvJEtiAaIfBDzJacVsbyYegMHZ0qpcd3PKQ44lC5GfYp7Z B4mStvhZfJhA2ETNlZrXBxQrj+8h2kQ7xtDk0=*Domainkey-signature*: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; b=BSz2Z2Z9oAaX/FrgAzAMD2HwQr581N27QWgj2eU0k32R0pjn5y1zpXSiefeniCLTgN zv+SH5ycESDQ1I+LSg8yZ98/mz8Vsg8OZ4g8Ax8Qcn7+/UpGXdJzmQYOpdiUrxg6zTi2 o+i5frwJVhJ+2sBKo92UCuLLBpyFKRSFUUk8o=

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 >

**Follow-Ups**:**Re: [eigen] Mapping array of scalars into quaternions***From:*Mathieu Gautier

**References**:**Re: [eigen] Mapping array of scalars into quaternions***From:*Mathieu Gautier

**Re: [eigen] Mapping array of scalars into quaternions***From:*Benoit Jacob

**Re: [eigen] Mapping array of scalars into quaternions***From:*Mathieu Gautier

**Messages sorted by:**[ date | thread ]- Prev by Date:
**[eigen] Re: build system changes -- wrap up** - Next by Date:
**Re: [eigen] Mapping array of scalars into quaternions** - Previous by thread:
**Re: [eigen] Mapping array of scalars into quaternions** - Next by thread:
**Re: [eigen] Mapping array of scalars into quaternions**

Mail converted by MHonArc 2.6.19+ | http://listengine.tuxfamily.org/ |