RE: [boost] Review: Special functions, Quaternions, Octonions

From: Gary Powell (Gary.Powell_at_[hidden])
Date: 2001-05-18 10:15:15

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"

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

  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.

  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.

  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.

  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?)

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

  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.

  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.

  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.

  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.)

  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.

  Yours,
 -gary-

gary.powell_at_[hidden]