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

[Jonathon Jongsma]

On Mon, 2018-03-26 at 12:36 +0200, Christophe de Dinechin wrote:
> 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_s
> > > > > > trin
> > > > > > 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.

Well, you're right that the interfaces are not exactly the same since
std::runtime_error has an extra constructor that takes a const
std::string&. But I'm not actually using that constructor. As teuf
mentioned in a different email, std::runtime_error in C++11 has a const
char* constructor:

class runtime_error : public exception {
public:
  explicit runtime_error (const string& what_arg);
  explicit runtime_error (const char* what_arg);
};

And that's the interface that I was illustrating above. It is not
creating a std::string from the const char* and calling the
std::string constructor, it is calling the const char* constructor
directly.


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

I think you're kind of missing the point again. The point is that
std::runtime_error is a *standard library class*. The fact that your
error supports nullptr, but runtime_error does not is irrelevant.
People are (theoretically) very familiar with the standard library and
what sort of usage those classes can support and what sort of errors
you need to be careful of. If you introduce a new class that mimics a
standard library class but causes a segfault when used in the same
manner as the standard class, that new class is likely to be misused
and is therefore "error-prone". It doesn't matter whether your
interface is objectively better than the standard library interface or
not. It only matters that it is different.


> 
> 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();

I don't think this is really relevant to my point. See above.


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

I disagree, see above. I'm using the const char* constructor.

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

You can still require const char* arguments, but copy the string and
store it within your Error class. I'm not arguing that you need to add
a std::string constructor. I'm only suggesting that you should not rely
on the caller to keep the pointer valid until the exception is caught
(as std::runtime_error and all other standard library exception classes
do).

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