[Spice-devel] [PATCH 12/22] Add exception handling classes

[Christophe Fergeau]

On Fri, Mar 02, 2018 at 02:00:23PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 2 Mar 2018, at 09:03, Frediano Ziglio <fziglio at redhat.com> wrote:
> > 
> >>> 
> >>> On 1 Mar 2018, at 13:13, Frediano Ziglio <fziglio at redhat.com> wrote:
> >>> 
> >>>> 
> >>>> From: Christophe de Dinechin <dinechin at redhat.com>
> >>>> 
> >>>> Throwing 'runtime_error' directly should be reserved for the support
> >>>> library.  Add an 'Error' class as a base class for all errors thrown
> >>>> by the streaming agent, as well as subclasses used to discriminate
> >>>> between categories of error.
> >>>> 
> >>>> The Error class offers:
> >>>> - a 'format_message' member function that formats a message without
> >>>> allocating memory, so that there is no risk of throwing another
> >>>> exception at throw time.
> >>>> 
> >>> 
> >>> why not formatting the message constructing the object so before the throw?
> >> 
> >> That requires dynamic allocation, which is entirely unnecessary and
> >> potentially dangerous (replacing one exception kind with another).
> >> 
> > 
> > As explained by my string.c_str example your interface is prone to errors.
> 
> The interface is not “prone to errors” at all. The scenario you
> describe makes no sense, since the whole interface is designed to
> avoid dynamic strings. This is now explicitlyi documented (see
> https://github.com/c3d/spice-streaming-agent/blob/c%2B%2B-refactoring/include/spice-streaming-agent/errors.hpp,
> not yet shared, I’m not entirely done with the reviews). The
> constructor comment now says:
> 
> 	Constructor takes the base message returned by what()
> 	The message must remain valid over the whole lifetime of the exception.
> 	It is recommended to only use static C strings, since formatting of any
> 	dynamic argument is done by 'format_message' 

This behaviour seems inconsistent with std::runtime_error(const char *)
which as far as I can tell does not have this lifetime expectation.

> 
> Anyway, your example is misleading. Anybody that passes the result of
> string::c_str() to a const char * without proper consideration for the
> lifetime is at fault. The const char * interface itself cannot be
> faulted for that.

For what it's worth, I usually expect const char * interfaces to make
their own copy of the const char * arg if they need it after they
return, and I've rarely seen exceptions to this expectation.

If I understood properly, things are designed this way to avoid running
out of memory while building the exception. I'm definitely not in favour
of having an error-prone API to handle a very rare case which the rest
of the code is most likely not going to be able to cope with anyway.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180302/142cdc29/attachment.sig>