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

[Jonathon Jongsma]

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.

> 
> 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. That inconsistency is the
thing that makes it easy to make errors.


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