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

[Christophe de Dinechin]

> On 7 Mar 2018, at 12:51, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 
> On Wed, Mar 07, 2018 at 12:28:57PM +0100, Christophe de Dinechin wrote:
>>> This behaviour seems inconsistent with std::runtime_error(const char *)
>>> which as far as I can tell does not have this lifetime expectation.
>> 
>> You are correct, but please note that std::runtime_error takes a string as input argument, making it very clear that you are dealing with a ‘string’ underneath, not a const char*.
> 
> Nope, it accepts both in c++11

I know that. (Quizz: can you explain why?)

The point was that it has a string constructor. So you don’t get an error if you use a string as an argument.

> http://www.cplusplus.com/reference/stdexcept/runtime_error/
> And the const char * version will make a copy of the const char * you
> give it...
> 
>> 
>>> 
>>>> 
>>>> 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.
>> 
>> std::vector and all standard library containers? You do a push_back of a const char * in an std::vector<string>, you get a copy. If you do that in an std::vector<const char *>, you don’t.
> 
> I'd argue templated code is special.

Why?

(I can probably find non-template examples, but I’d like to understand your reasoning first)

> 
>> Let me give an example from *our own code*. ConcreteConfigureOption
>> takes two const char * arguments. Does it make copies? No. Same thing
>> with the related AddOption method. Anything particularly surprising
>> there? I don’t think so.
> 
> Hmm, in my opinion it should make a copy... Given what it's used for,
> it's highly likely it's going to be used with static strings, so why
> not.

What makes options more “likely” to be used with static strings than the Error constructor? We are talking about a class designed to store error messages. Anything more static than that?


> But I really would not use that as a generic example which should
> be followed throughout the codebase ;)

I am not. I am just pointing out that your expectations are not met in our own codebase.


> 
>>> 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.
>> 
>> To make this clear, let me elaborate a little on *one* other reason (I
>> listed a few others), code bloat and runtime cost. Let’s say we want
>> to store a write error with an errno. The two approaches under
>> consideration are:
>> 
>> 	A. throw WriteError(message, operation, errno);
>> 	B. throw std::runtime_error(message + “ in “ + operation + “: “ + strerror(errno))
>> 
>> I’ll briefly mention, as a gentle reminder of some of the “other reasons”, that (A) is
>> 1. shorter to type
>> 2. easier toi read
>> 3. less error prone
>> 4. more likely to guarantee consistent messages
> 
> No problem with having A as an API, but I don't think achieving A
> mandates that we don't generate the string at the time the exception is
> created. This really is what is being discussed at this point.

Well, I’m glad I convinced you so far, it was not quite clear until now. I will stop arguing that then.

So then my next question is: if there is a message formatting approach that does not require dynamic memory allocation nor throws exceptions, why settle for less?

> And if we are optimizing exception creation from the start, without any
> profiling data saying we have to do this, I'd say we are falling into
> very premature optimizations…

From my point of view, you are requesting a large number of severe belated pessimizations. Belated because I already did the optimization, and you are asking me to remove it after having actually implemented it. Pessimizations because you are asking to make the code worse, and you seem to be aware of that. It’s often said that “premature optimization is the root of all evil”. Len Lattanzi liked to point out that “belated pessimization is the leaf of no good”.

Anyway, I could go with de-optimizing if we gained something from it, if it was an actual trade-off. But here, there is no trade-off. It’s replacing a win-win with a lose-lose. In addition to making the code less efficient, it would encourage programmers to pass dynamic strings. This is an Error class. Its message are by definition error messages. They are supposed to be static constants, by design. String concatenation in error messages is widely acknowledged as evil.

I could have selected a different design, e.g. passing a message ID from a message catalog the way old MacOS applications used to. In Linux, traditionally, the key for gettext() is the English error message. So I stuck with that. It’s still an error message, not some random “Hello world” dynamically concatenated string, not some string I get from a C API that returns a dynamically allocated string. That really did not seem like a far-fatched, outlandish concept to me, certainly not one worth arguing over for one month.

> 
> So... Can we make the API of our exception classes safer to use as requested multiple time?

They are safer in their present form than in the form you are advocating, since they detect the common case of someone trying

	throw Error(“unable to write “ + filename);

instead of taking the time to analyze the structure and semantics of the message and turn that into something like

	throw FileWriteError (“unable to write file”, errno, filename);

The last part correctly builds an exception object which is easy to catch individually, has the dynamic data carefully put aside, can be localized, can be converted into some other message with additional information, and all the good things I already pointed out. If the filename is dynamic and you want to put an std::string for that, that is perfectly fine with me (as long as you don’t suggest ever again to provide some ad-hoc alternative implementation of std::string using strdup, which would be the worst way to do it). But the message is not a dynamic string, and should not be; it is as much a compile-time constant as anything can be. That is entirely intentional and by design.


Christophe

> 
> Thanks,
> 
> Christophe