Re: Review: Special functions, Quaternions, Octonions

From: Hubert HOLIN (Hubert.Holin_at_[hidden])
Date: 2001-05-18 16:34:30

Paris (U.E.), le 18/05/2001

--- In boost_at_y..., Gary Powell <Gary.Powell_at_s...> wrote:
> Comments:
> 
>   First, let me admit, I don't know the math behind these classes, so I
> won't be able to judge that at all.
> 
>   Second, the code is pretty easy to read, and I have a desire to approve
> this library, merely because I have friends who need it, and have written
> their own. So with the caveat, I haven't run the test code, I vote APPROVED.
> 
> So now I have some nit/picky stuff: (being the kind of detail guy I am.)
> 
>   #include <boost/filename>
> 
> should be
>    #include "boost/filename"

        Well, this is the way it was, but the change to <...> was 
requested by Jens Maurer during the early phases...

>   class octonion:
>   default constructors, and constructors taking one argument, with the rest
> default, should be declared "explicit"

        I disagree in the case of the default constructor. Why would you 
want them in the other cases you mention?

>   The assignment operator=(octonion const & a_affecter), has an
> optimization,
> if (this != &a_affecter), since there are no pointers in this class, and IMO
> programmers rarely write a = a, and since doing so would not create an
> illegal copy, this test should be removed.

        In Murphy I trust... Though if this is the consensus, I have no 
objection to removing the "optimization" (which usually is a slow-
down...).

>   Also, while I can see that it would not generate the most efficient code,
> I have a tendency to write templated assignment operators as
> 
>   octonion temp(a_affecter);
>   using std::swap;
>   swap(temp, *this);
>   return this;
> 
> which is exception safe, but makes an extra temporary copy. So I don't know
> whether the speed vs safety would make a difference in actual usage. But
> since you are specializing, float, double, and long double, you can do the
> fast thing there and the more general thing in your basic class.
> 
>   For all operators like octonion<T> operator /= (T const &rhs), shouldn't
> these be member templates, as in
> 
>   template<typename S>
>    octonion<T> operator /= (S const &rhs)
> 
> which should produce legal code where a T = T / S;   is a legal expression.

        I'd rather keep my distance with "swap" until we clearly know 
where to put it.

>   Shouldn't the general case of R_componet_N() const, return a const T &?
> obviously for the float, double and long double a copy is fine.

        For built-ins this is fine, and for the general case I just kept 
the same kind of signature. The problem is that there is no realy 
usefull and relevant way to test the unspecialized template to see what 
is really necessary in that case (what I feel is necessary is a fixed 
point class, but in a sense which is quite different (because the 
domain problem is different) from the one which is currently being 
discussed in another thread). In any case, if the "const T &" form is 
prefered, I will change the signature of all these functions to it.

>   Also I have a general question, that someone in boost may know the answer
> to, and that is, "Is it possible to just specialize the member functions
> that you care about?" In which case, for float, etc. could you just
> specialize those member functions for which you have optimized the operator.
> (Or is that not worth it?)

        Unfortunately no, template specializations may be essentially 
unrelated.

>   At some future time, you might consider using expression templates for
> those operators for which it makes sense. (I'm thinking addition and
> subtraction)

        Could you please give more details on that?

>   Also you may want to consider adding your own manipulators for setting the
> '(', ')' and ',' delimiters for io. There are some good articles over on the
> CUJ expert forum site that tell you how to do this.

        Any URL welcome...

>   Also the code for operator >> appears to be able to use a subroutine or
> two, or am I missing something about the ODR that would kill it. (You fake
> it by adding a class that had a member function that did the actual work.)
> But validating 1,700+lines of text and code seems a daunting task, and one
> that could easily contain a bug or two.

        This operator is a godawfull mess... But I was indeed worried 
about the ODR, and much prefer to have a header which provides all the 
functionality by itself.

>   I would also prefer that this package have its own subnamespace, rather
> than be just in boost. i.e. "boost::quaternions" or whatever. At least the
> special functions should be there, as if you were to swap "boost" for "std"
> then there wouldn't be name collisions with "atanh" et.al.

        If boost delineates namespaces, I will gladly go along with them.

>   The special functions, "atanh" etc. take a T copy instead of a T const &
> as IMO it should, as it does not appear to modify the argument. While it
> appears that these functions are intended to be used with "float" etc. they
> could be used with a user type T for which a specialization of std::log,
> std::abs, std::sin has been written. (I'm not suggesting that I'm actually
> going to do this.)

        This is a contious decision. My feeling is that one way or another 
C will define a broader coupus of standard special functions. I want to 
be able to use that body of work (with C++) when it will be there. 
Hence for built-in types the function signature has to be a C-style 
signature, compatible with those that are already available (sin, cos, 
exp...). But then that forces my hand for more complex types, such as 
complexes, quaternions and octonions. At any rate anyway, these 
functions will be overwelmingly used with built-in types, so what 
favors them should be prefered.

>   I have mixed thoughts about this next suggestion, and that is to implement
> a member function "swap" and a specialization in the namespace "std" of
> swap. Partly because I'm not sure that for the types, float, etc it really
> makes much difference, and partly because for a user defined type there are
> already a host of issues with std::swap etc. (there is plenty of previous
> discussion and I don't really want to restart that.)
> 
>   So looks like a great addition to boost to me.

        Thanks!
> 
>   Yours,
>  -gary-
> 
> 
> gary.powell_at_s...

        Well, once I feel more confortable with swap...

Yours truly

                Hubert Holin
                Hubert.Holin_at_[hidden]