Re: [boost] [review] Exception

From: Emil Dotchevski (emil_at_[hidden])
Date: 2007-10-18 15:47:26

Pavel,

First of all, thanks for the detailed review.

> 1. exception/exception.hpp (btw, this file name can easily confuse one with
> the same filename one level down) - assert() may be replaced with
> BOOST_ASSERT or even better commented out. People may use their own assert
> implementation and this would make the exception module (something what
> should be really the core part) dependent on something else.

If there is a requirement for boost libraries to use BOOST_ASSERT,
I'll make that change if the library is officially accepted in Boost.

> 2. shared_ptr<> may be replaced by intrusive_ptr<> or home made equivalent.
> This (a) reduces dependency on other Boost part (shared_ptr does depend on
> quite few other libs) and (b) the atomic locking used by shared_ptr would be
> eliminated. This would help a little bit on multiprocessor systems - lock
> may be hundredths of cycles and during the time access to the memory bus is
> disabled.

(a) is a valid concern, OTOH shared_ptr is such a low level component
of Boost that -- as careful as I am in avoiding physical coupling -- I
don't consider it a real dependency; rather, it's a tool for avoiding
dependencies.

(b) can be addressed if someone reports having performance issues with
Boost Exception. This is highly unlikely since we're comparing the
time it takes to copy a single shared_ptr once (at the time of the
initial throw) vs. the time it takes for the implementation to unroll
the stack until a suitable catch is found.

> 2. The documentation may say whether it is possible to use multiple
> inheritance with boost::exception descendants and show how. I tried it some
> time ago, it didn't compile so I gave up.

With this regard, boost::exception is basically the same as
std::exception, which is why I didn't bother to elaborate on how to
use it as a base class, but at least I should point out these
similarities in the documentation; good point.

> 1. As mentioned above, the ability to iterate through values and/or pass a
> visitor.

Iteration presumes that you catch a boost::exception knowing nothing
about the semantics of the actual exception type that was thrown, yet
you want to make use of the data it contains.

One use case would be to log all data contained in an (unknown)
exception, for debugging purposes; this is supported by
boost::exception::what().

Another use case is when you want to catch exception type A and throw
exception type B instead, such that the B object contains all the data
from the A object plus some additional information. However, one of
the motivations for Boost Exception is that it helps avoid the need to
translate exception types, since you can catch A as boost::exception
&, add whatever relevant data you have, and then re-throw the original
A object.

> 2. Cloning of the exception. When I catch exception thrown in a worker
> thread I create clone of it and throw it in the thread which requested the
> task to be executed in separate thread. It looks like:
>
> struct my_exc : public boost::exception
> {
>   virtual my_exc* clone() { return new *this; }
> };
>
> I'd created common abstract subclass because of it, perhaps it would make
> sense somehow to support the feature.
>
> I see the http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2179.html
> now but it doesn't help me here and now, I think.

I believe that cloning exceptions is outside the scope of Boost Exception.

> 3. My code often looks like:
>
> throw exc::some_exception() << boost::error_info<exc::tags::my_tag1>(errno)
> << boost::error_info<exc::tags::my_tag2>("this") <<
> boost::error_info<exc::tags::my_tag3>("that") ;
>
> IOW, very long and hard to read lines. I would welcome some way to fill in
> the exception in more compact form. I, however, do not have any suggestion
> how to do it.

There was a suggestion to support grouping of separate data that is
always added together to a boost::exception at the time of the throw.
As Peter pointed out, this support can be implemented on top of the
proposed design. Another possibility -- which would be my choice -- is
to simply put such data into a struct that gets added to
boost::exception as a single object.

> 4. I would like the ability to collect traces generated by what() function
> in DEBUG mode, something as:
>
> catch (my_low_level_exception& e) {
>    my_high_level_exception e2;
>    ... fill in e2
>    e2.add_debug_trace(e);  // adds e.what() somewhere, no-op in release mode
>    throw e2;
> }

Would this work for you:

struct tag_debug_trace: boost::error_info<std::string> { };

void add_debug_trace( boost::exception & e2, boost::exception & e )
{
#ifdef _DEBUG
    if( !boost::get_error_info<tag_debug_trace>(e2) )
        e2 << boost::error_info<tag_debug_trace>("");
    (*boost::get_error_info<tag_debug_trace>(e2)) += e.what();
#endif
}

and then:

catch (my_low_level_exception& e) {
   my_high_level_exception e2;
   ... fill in e2
   add_debug_trace(e2,e);
   throw e2;
}

> 1.  What happens if a "boost::error_info" tag is used multiple times on
> the same object?  Is it an error (exception or assert?), do the
> instances get combined like a multi-set, or do all instances besides the
> first (or last) get ignored/dropped?  Shouldn't this be documented?

Storing data under a tag overwrites the previous data stored under the
same tag. Yes, this behavior should be documented.

> 2.  Shouldn't all inheritance of "boost::exception," especially via
> "enable_error_info," be of the virtual kind?

This is another thing I didn't pay much attention to in the
documentation, because deriving boost::exception is the same as
deriving std::exception.

As for enable_error_info, I am planning to change it to not inject
boost::exception as a base if it already is a base. This would allow
enable_error_info to be used in boost::throw_exception regardless of
whether a particular exception type derives from boost::exception, or
not. This nullifies the concerns whether enable_error_info uses
virtual or non-virtual inheritance.

> About the compile safety of tags: I prefere to have them statically checked.
> Acesibility of the tags could be limited only for relevant parts of an
> application thus removing chance to depend on such internal details in
> urelated code. String based indentification, if used, should be an option,
> not mandatory.

I agree with your preference for compile-time type safety. If there is
a strong expert opinion that string-based identification is
preferable, we could switch to string-based identification but in that
case I would drop the tag-based identification altogether.

> It would be ideal, if the library is accepted into Boost that all currently
> thrown exception from Boost libraries (e.g. archive_exception from
> Serialization) descend from boost::exception. (I am bot holding my breath,
> this is just an unreachable ideal.)

Boost Exception is most useful when you communicate with 3rd-party or
user code that throws exceptions that derive from boost::exception.
This would only be possible if boost::exception becomes an universal
base class for exception types, much like std::exception is, which is
why I've made this library available for consideration as a Boost
library.

If Boost Exception is accepted to Boost, I wouldn't call making all
boost exceptions derive from boost::exception unreachable ideal; in
fact enable_error_info is a straight-forward way to reach it.

Emil Dotchevski