Re: [boost] uBLAS formal review

From: Fernando Cacciola (fcacciola_at_[hidden])
Date: 2002-06-27 09:41:46

Hi,

I started the evaluation by scanning some headers while I was compiling some
of my projects (with my TurtlePC...)

This is what I spotted already:

traits.h
~~~~~

IMO, the generic math functions 'abs' and 'sqrt' must be provided by a stand
along general header.
The problem with the current approach is that it uses a single config macro
(NUMERIC_C_MATH) to figure which compiler-supplied function to use.
Unfortunately, the world is not that easy. Some compilers (i.e. BCC) provide
some of the functions but not others, as single all-or-nothing approach just
won't work

Eric Ford, Kevin Lynch and myself started to work on a generic math layer
which is intended to cover this issue in general. Unfortunately, non of us
had the time to finish it, but it was nearly complete. Noah Stein worked on
the VC6 port but got stuck somewhere.
The main layout is already established, and it mainly remains to plug
in each compiler-specific code.
This work can be found at:
http://groups.yahoo.com/group/boost/files/standard_functions/std_math/

Naturally, right now uBLAS would stay as it is, but this would *have to*
change
as soon as possible because the current implementation won't work for
all combinations of supported-compilers/fp types.

concepts.h
~~~~~~~~

The first concepts are those already supplied by the concept_check.hp
header.
These should be removed from this file and concept_check.hpp included
instead.

This header defines class templates such as:  Vector<> {}, Matrix<> {},
etc..
These are Concepts, but they live in boost::numerics:: and are named as if
they were 'vectors' and 'matrices' themselves.
Their names don't reflect the fact that they are concepts. I think they
should be called VectorConcept, etc... (Actually, I think that most of the
classes in this header should be postfixed "Concept")

Similarly, there are function templates called zero<>(), one<>(), etc...
These should be in a detail namespace; and even then, called differently,
because that prevents a non-template overloaded set of functions to be
called zero(), even on another namespace (at least that's the case with
BCC55).

The RingWithIdentity concept is a model of AdditiveAbelianGroup, but it is
also a model of MultiplicativeAbelianGroup even though this isn't not
reflected by the concept constraints().

exception.h
~~~~~~~~
I think that the custom exception classes should derive from the
conceptually closer standard exception, not from std::exception.
For example, divide_by_zero should derive from std::runtime_error,
internal_logic from std::logic_error, etc...

Now a suggestion: There is a very significant issue regarding exceptions and
function inlining. Most compilers won't inline functions throwing
exceptions, not even if the throw expression appears in a nested call if
this nested call is also inline (This was pointed out to me once by Gennadiy
Rossental).
The solution I used, which AFAICT, has significant runtime impact because it
increases actual inlining, is to let the exception classes have an out of
line function 'raise()' so that throwing an exception of type 'e' is done
using 'e.raise()' instead of 'throw e'.
This is specifically important in tiny inline functions which uses exception
in preconditions.
Additionally, as I said in another message not long ago, this scheme allows
the 'user' of the library to choose whether to actually throw an exception
or to abort() in case of an assertion failure without massive recompilation.
It only requires that 'exception::raise()' be able to use some global flag
or the like to decide what to do.

OK, I'll go on with the other headers as I get some more time...

General Notes:

Some headers (functional.h), for instance, use include directives of the
form: #include "xxxx.h".
These should be carefully looked up and change to #include
"boost/numerics/ublas/xxx.h"

I notice that some headers and some jamfiles contain explicit directives to
supress warnings. IMHO, this is kind of cheating, actually. I would prefer
if warnings were removed with appropriate coding instead, whenever possible
at least.

Tests:

I tried to compile test1 with bcc5.5, but I get this error while parsing
test12.cpp:

matrix_et(559): E2034, Cannot convert 'const std::complex<float>' to 'int'.

The same error appears in this header at lines: 597 and 616.

So I'm stuck...

Fernando Cacciola
Sierra s.r.l.
fcacciola_at_[hidden]
www.gosierra.com