Re: [boost] uBLAS formal review

From: Joerg Walter (jhr.walter_at_[hidden])
Date: 2002-06-27 17:56:50

----- Original Message -----
From: "Powell, Gary" <powellg_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Thursday, June 27, 2002 11:45 PM
Subject: RE: [boost] uBLAS formal review

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

:-)

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

Ok.

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

Sometimes 'friend' signatures are simpler.

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

MSVC has _forceinline.

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

Already done due to a request of Michael Stevens (at least in my private
version ;-).

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

Ok, I'll try to fix.

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

Ok, I'll try to fix.

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

We'll have to optimize the iterators.

> Design:
>   is boost::numerics::vector designed to be a replacement for std::vector?

No.

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

Who hijacked the name? ;-)

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

That's the rationale.

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

It usually doesn't seem to improve the performance much.

>   Should the include files be "filename.hpp" ? to fit the rest of boost.

Coming from MSVC, I'm unsure (but I think we'll change and possibly lose CVS
history).

> Also shouldn't the
> include path be
> "boost/numerics/filename.hpp" ?

Here I'd like to hear other opinions.

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

Again, here I'd like to hear other opinions.

Regards

Joerg