Re: [boost] uBLAS formal review

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

----- Original Message -----
From: "Fernando Cacciola" <fcacciola_at_[hidden]>
To: "boost developers" <boost_at_[hidden]>
Sent: Thursday, June 27, 2002 4:41 PM
Subject: Re: [boost] uBLAS formal review

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

Definitely.

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

Agreed.

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

Ok.

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

Ok, although concepts.h isn't intended to be used in client code.

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

Ok, although concepts.h isn't intended to be used in client code.

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

Not exactly, some problems with the inverse.

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

Ok.

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

I'll look into that.

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

Already done (at least in my private version ;-).

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

The disabled compiler warnings usually are really dumb (like BCC telling us
'can't inline this function because it uses ...'). I'm rather sure, we're
not cheating here.

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

Sorry for breaking the Borland build. I've just rechecked that. Please
change the offending lines from something like

    return const_iterator1 (*this, it1, it2, it2 != it2_end ? *it2 : 0);

to something like

    return const_iterator1 (*this, it1, it2, it2 != it2_end ? *it2 :
value_type ());

Regards

Joerg