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

[Christophe de Dinechin]

> 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_string<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?

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"?


>    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.