RE: [boost] uBLAS formal review

From: Powell, Gary (powellg_at_[hidden])
Date: 2002-06-27 16:45:52

First, let me say this looks very impressive. Second, I'm basing this first set of comments only on reading the code and documentation. I'm not sure whether I have enough time to fully evaluate this library but I do have some comments based on my reading so far.

Anyway I vote for inclusion. The code looks clean, the documentation seems to explain what's up. Looks like a lot of work was put into it.

  -Gary-

Specific comments below.
-------------------------------------

code:
exception.h
  It seems to me that some of the exceptions are "runtime_error" (bad_size, bad_index) and some are "logic_error" (internal logic, external logic) and should be derived from the other than std::exception.

use of
   friend void swap(A &a, A&b)
   {  a.swap(b); }

should be a standalone function instead of a "friend", no need as it doesn't reference private data. (There are a number of examples of this.) 

  Lots of "NUMERICS_INLINE" I thought this was useless, as in compilers don't pay any attention to this if you define the member fn inside of the class definition. I'd either stop using the define, or move the fns definitions outside the class declarations if you really intend to have this as a compile time switch.

vector_range::iterator & const_iterator,
  I think it would be worth it to provide operator++(int) and operator--(int) (postfix increment and decrement)

  Shouldn't operator[](size_type i) const return a const_reference instead of a value_type?

  Shouldn't vector_range::const_iterator::operator*() const return a const reference instead of a value_type? 

General coding:

  (As seen in banded.h)
  Since performace not readability is key, shouldn't
  i = ::std::max(i, value);
 just be
  if (i < value) i = value;
?
  I would think it would save one store instruction. (i = i; )

Design:
  is boost::numerics::vector designed to be a replacement for std::vector? That two "vectors" exist both seems good and bad, ie. with namespacing the names can be reused, on the other hand do we really need two vectors? I suppose this is more of a C++ standards committee issue, and acceptance of this library should not depend on it one way or the other.

  c_vector::resize() throws "bad_alloc" I tend to think of this as "out of memory" rather than a prohibited call. I would think that a different error would make more sense. Although I could see that user's might only expect bad_alloc and hence only catch that one.

duff's device,
  I tried this once or twice and each time performace was worse as with the loop everything appeared to be in the cpu cache, with duff's device it ended up swapping the code in making it considerably slower. Have you done any performace testing on this? (Did I miss it in the docs?)

  Should the include files be "filename.hpp" ? to fit the rest of boost. Also shouldn't the include path be 
"boost/numerics/filename.hpp" ? doesn't that prevent more name conflicts? It's been so long since I've had this issue come up.

  Namespace, I'm torn whether it should be boost::numeric:ublas.... or boost::numeric::... as there may be more "vector" and matrix implementations coming along and we should be able to accomodate them.