Re: [boost] [review] Review of Boost.Exception

From: Dean Michael Berris (mikhailberis_at_[hidden])
Date: 2007-09-30 15:55:26

Hi Emil!

On 9/30/07, Emil Dotchevski <emildotchevski_at_[hidden]> wrote:
> > get_error_info(T const &) returns a pointer instead of a const copy of
> > the actual error_info it gets. This can be a potential headache to
> > deal with especially in examining the error_info associated with the
> > thrown exception. A proposed change would be to have get_error_info(T
> > const &) return a const copy of the data, while get_error_info_ptr(T
> > const &) would return a pointer as in the current version.
>
> I suppose the behavior you propose for get_error_info is to throw an
> exception if the requested data is not available in the exception. But
> think about the situation you're describing: you catch an exception,
> and now you want to query it for error_info -- what good does it do
> for that context to throw an exception?
>
> Sure, there are use cases when you know an error_info must be present
> in the exception or you have a bug, but in that case it makes more
> sense to assert on get_error_info.
>

Far from it. I'm thinking get_error_info(T const &) should not compile
if the error_info being requested has not been passed to the
exception. This would only be possible if the exception type itself is
composed using expression templates and its type information is
somehow reflected during runtime -- or it might be impossible given
that the exception is referred to via an abstract base `boost::error`.
Maybe an encapsulated tuple that "grows" along the exception
propagation might work though I'm not entirely sure about that as
well.

At any rate, perhaps using something like Boost.Optional as the return
value (or an additional parameter to get_error_info) mitigates the
problem you describe. Somehow, for me the following reads a bit more
coherent if not safer:

try {
  ...
} catch (boost::exception & e) {
  boost::optional<int> errno = boost::get_error_info<tag::errno>(e);
  if (errno) { // means errno is set
    cerr << *errno;
  } else { // means errno is not set, then set it
    e << boost::error_info<tag::errno>(-1);
  };
  throw; // propagate exception
};

This way, I cannot easily circumvent the pointer returned and mangle
it perhaps to further mess things up. Not that I'm saying anybody
would do this, but since someone could do it, then it's easier to have
users shoot themselves in the foot instead of providing an interface
that's considerably safer than pointers.

Not that there's anything bad with pointers, it's just too easy to get
them wrong that it's usually a good idea to stay away from them as
much as you can. That's what I think at least.

> > Although it might also take quite some effort to remove the runtime
> > polymorphic behavior which relies on shared_ptr<>'s and virtual tables
> > with info_base <snipped>
>
> I never had any performance-related issues with this library, and I've
> been using it for more than a year now. Until we have evidence that we
> need to optimize, I would prefer to keep the implementation as simple
> as possible.
>

It's good to know that you haven't had any performance-related issues
with this library. I'll take your word for it.

However, a shared_ptr<> implies allocating something on the heap.
Anytime you make an (arguably) unnecessary memory allocation in the
heap, in my book is a performance affecting issue. We can go on and
argue about long jumps, cache misses, and memory locality issues, but
this would be moot since the goal of the library is primarily to
provide a functionality that seems to require the heap allocation *by
design*.

So I agree, unless there's evidence that shows it's a performance
issue (having shared_ptr<>'s and runtime polymorphic behavior in
exceptions) then I can't convince you that shared_ptr<>'s might be
needless and that there might be a better way of doing this. I'll also
say I wouldn't even try. ;-)

> > My concern with the use of shared_ptr<>'s is that in cases where an
> > exception pertaining to "out of memory" conditions would arise,
> > throwing a boost::exception and then having to allocate more memory
> > just to encapsulate additional information would lead to undesirable
> > termination.
>
> Could you elaborate a bit more? Can you come up with a theoretical
> example that demonstrates your concerns?
>

Theoretically, (or I think even in reality) a call to the (default)
new operator may trigger a bad_alloc exception. This exception may be
caught, and somewhere along the way code decides to encapsulate the
information in that bad_alloc exception into a boost::exception -- and
chooses to say where that exception happened, as a tag in the
propagated boost::exception.

So having additional heap allocations when an out of memory exception
is already thrown doesn't make a lot of sense mainly because you've
already run out of memory that's why the exception is thrown in the
first place.

Although fixing bad programming practice is not within the design
goals of the proposed Exception library, making it harder for users to
use it in a wrong manner would be a nice goal IMHO.

> Thanks for you feedback!

You're welcome. :)

-- 
Dean Michael C. Berris
Software Engineer, Friendster, Inc.
[http://cplusplus-soup.blogspot.com/]
[mikhailberis_at_[hidden]]
[[phone]]
[[phone]]