Boost logo

Boost :

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.


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


> 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

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



Boost list run by bdawes at, gregod at, cpdaniel at, john at