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

[Christophe de Dinechin]

I realize I had left this one unanswered.

> On 7 Mar 2018, at 15:14, Jonathon Jongsma <jjongsma at redhat.com> wrote:
> 
> On Wed, 2018-03-07 at 12:34 +0100, Christophe de Dinechin wrote:
>>> On 6 Mar 2018, at 17:47, Jonathon Jongsma <jjongsma at redhat.com>
>>> wrote:
>>> 
>>> On Tue, 2018-03-06 at 11:02 +0100, Christophe Fergeau wrote:
>>>> On Fri, Mar 02, 2018 at 06:06:32PM +0100, Christophe de Dinechin
>>>> wrote:
>>>>>> FWIW, I also find myself in agreement with Frediano and Lukas
>>>>>> on
>>>>>> this point.
>>>>> 
>>>>> OK. Maybe I misunderstood something.
>>>>> 
>>>>> Do we agree that the case Frediano raised is someone trying:
>>>>> 
>>>>> 	throw Error(“The value of “ + std::to_string(2) + “
>>>>> does not
>>>>> match “ + std::to_string(1));
>>>>> 
>>>>> which yields the following error:
>>>>> 
>>>>> 	spice-streaming-agent.cpp:165:93: error: no matching
>>>>> function
>>>>> for call to
>>>>> ‘spice::streaming_agent::Error::Error(std::__cxx11::basic_strin
>>>>> g<ch
>>>>> ar>)’
>>>>> 
>>>>> So far, I think the interface was not “error prone”. It caught
>>>>> the
>>>>> error correctly.
>>>>> 
>>>>> Therefore, in order to find the interface “error prone”, one
>>>>> has to
>>>>> accept a second hypothesis, which is that whoever attempted
>>>>> that,
>>>>> after seeing that error message, looks at the interface, which
>>>>> has
>>>>> a ‘const char *’ and a comment that specifically states "It is
>>>>> recommended to only use static C strings, since formatting of
>>>>> any
>>>>> dynamic argument is done by ‘format_message’”, and after
>>>>> reading
>>>>> that comment, has a big “Aha” moment and writes:
>>>>> 
>>>>> 	throw Error((“The value of “ + std::to_string(2) + “
>>>>> does not
>>>>> match “ + std::to_string(1)).c_str());
>>>>> 
>>>>> I’m sorry, but I cannot accept that step of the reasoning as
>>>>> being
>>>>> even remotely valid. Just looking at the code makes me want to
>>>>> barf.
>>>>> 
>>>>> So either I misunderstood Frediano’s point, or the reasoning is
>>>>> deeply flawed in a not very subtle way.
>>>> 
>>>> It's not just about .c_str(), it's really about any API returning
>>>> a
>>>> non-static string which will be freed before the exception is
>>>> caught
>>>> (think RAII-like cases, wrappers around preexisting C APIs, ...).
>>>> I already mentioned it, but an API taking a const char *, and
>>>> also
>>>> mandating that this const char * must be alive as long as the
>>>> instance
>>>> I invoked the method from is really unusual/unexpected to me, and
>>>> would
>>>> thus be error-prone.
>>>> Christophe
>>> 
>>> 
>>> A simple concrete example:
>> 
>> Your example is basically a variant of mine.
>> 
>>> ------
>>> 
>>> #include <exception>
>>> #include <iostream>
>>> #include <string>
>>> 
>>> class Error final : public std::exception
>>> {
>>> public:
>>>   Error(const char *message): exception(), message(message) { }
>>>   virtual const char *what() const noexcept override { return
>>> message; }
>>> protected:
>>>   const char *message;
>>> };
>>> 
>>> 
>>> void do_it(int x)
>>> {
>>>   std::string foo("Hello, world");
>>>   // do some other stuff...
>>>   if (x == 1)
>>>       throw Error(foo.c_str()); // dangerous
>> 
>> Yes. Why did you add c.str() to start with?
> 
> To show that it's possible to use c_str() with the standard exception class, but not with the new one.

The reason I was asking is because without c_str(), one example compiles, the other does not. This proves that the interfaces are *not* the same.

The initial point was that the Error(const char*) is supposedly error prone. If I take your example, but replace foo.c_str() with nullptr, another common error, then my variant works, and runtime_error crashes. So the “error prone” conclusion is only reached with an ad-hoc selection of which kind of error you focus on.

Furthermore, the use of c_str() in your example is the part that is unsafe. You don’t need the Error class to demonstrate that. As a proof that it is unsafe, you can simply write:

	throw foo.c_str();

That throws a pointer that is invalid at the catch point. So the problem is really with your use of c_str(), not with the Error class, since you don’t even need the Error class to get an invalid behavior.

> 
>> 
>> If I write this:
>> 
>> 	typedef std::vector<const char *> ArgumentList;
>> 
>> 	ArgumentList build_args_list(int argc, char **argv)
>> 	{
>> 		ArgumentList argList;
>> 		for (int a = 1; a < argc; a++)
>> 		{
>> 			std::string arg = argv[a];
>> 			// … do some other stuff
>> 			argList.push_back(arg.c_str()); // dangerous
>> 		}
>> 		return argList;
>> 	}
>> 
>> do I have a right to complain to the writers of the standard library
>> that their interface is “error prone"?
> 
> You didn't really address what I consider to be the important part of
> my message: it's an exception that has the same interface as a standard
> exception class, but behaves differently.

It does not have the same interface. One takes an std::string, the other takes a const char *. The fact that you can build an std::string from a const char * does not make any interface taking “const char *” “the same” as an interface taking “std::string”.

The reason my proposed interface takes const char * is that I really want that interface to be only called with “individually localizable english error messages as compile-time constant C strings”, but the C++ type system is not powerful enough to let me express that (in particular the “english” part of things), so I take the closest there is to it, and I add the rest in the interface documentation.

> That inconsistency is the thing that makes it easy to make errors.

No, that inconsistency is what leads to error messages, which is a desirable property. The error message, in turn, should make the developer realize that the intent and usage model is different from std::runtime_error, rather than attempt to work-around the error message by adding c_str() in a place where it’s remarkably dangerous to do so, even without the Error class.

> 
> 
>> 
>> 
>>>   else if (x == 2)
>>>       throw std::runtime_error(foo.c_str()); // fine
>>> 
>>>   std::cout << "run without any arguments for Error, or with 1
>>> argument for runtime_error";
>>> }
>>> 
>>> int main(int argc, char** argv)
>>> {
>>>   try {
>>>       do_it(argc);
>>>   } catch (Error& ex) {
>>>       std::cout << "Error: " << ex.what() << std::endl;
>>>   } catch (std::runtime_error& ex) {
>>>       std::cout << "runtime_error: " << ex.what() << std::endl;
>>>   }
>>> 
>>>   return 0;
>>> }
>>> 
>>> 
>>> ------
>>> 
>>> The fact that the interface is exactly the same as runtime_error
>>> but it
>>> behaves differently is almost the textbook definition of error-
>>> prone,
>>> IMO.