[Spice-devel] [PATCH 12/22] Add exception handling classes
Christophe de Dinechin
cdupontd at redhat.com
Wed Mar 28 10:17:33 UTC 2018
> On 26 Mar 2018, at 18:24, Jonathon Jongsma <jjongsma at redhat.com> wrote:
>
> 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:
You are artificially using the const char * constructor by adding a call to c_str().
My point the whole time has been that this makes the c_str() totally artificial, ad-hoc, contrived.
You are down-casting to the const char * type knowing that in the runtime_error case it’s re-converted back up to string (irrespective of the constructor you use), only to make the interfaces look similar when they are not. Which constructor is being used is totally besides the point.
It’s a bit as if I added an exp2(int) function doing a simple shift, and you advocated that should take a double input parameter because “exp()” does in the standard library, and illustrating your argument with:
void do_it(int x)
{
double foo = 42.0;
// do some other stuff
if (x == 1)
exp2((int) foo); // dangerous
else
exp((int) foo); // safe
}
My example looks silly, but it is unfortunately an exact parallel of yours:
- It downcasts the “foo” variable to a lower type (const char * in your example, int in mine).
- it selects a restricted case where that down-casting does not matter (“Hello World” and 42 are valid). Consider if foo contained “Hello\0World\0” in your example, and 42.6 in mine…
- It does a downcast that is necessary to shut up the compiler only in one case (exp2 or Error), but not in the other (exp or std::runtime_error)
- The conversions are inefficient (truncation in my example, memory alloc / copy in yours)
- They have side effects (number truncation in my case, \0 elimination in yours)
- Because of the last two reasons, no sane developer would add them that way.
You would be very concerned if I used this example to convince you that exp2() should take a double() like exp(). So please understand that I am very concerned that you insist with this example. I find it remarkably lacking in logical strength.
>
> 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.
I am very aware of that, and it’s totally irrelevant.
The key point is that you add c_str() totally artificially to make the interfaces look similar, when they are not.
>
>
>> 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.
It’s very relevant to the “error prone” theory. You can’t selectively focus only on those errors that are consistent with your “error prone" claim.
As for “standard library class”, I already pointed out that:
- std::exception or std::bad_alloc do not take a string argument, and they are standard library exception class as well.
- the behavior of most of the standard library, notably containers, is inconsistent with the const char * copy requirement you attempt to put on the Error class
Finally, a very important point is that Error is *not* a standard library class.
> People are (theoretically) very familiar with the standard library
Apparently not, given how widely mis-used it presently is.
So on one side, we have an actual mis-use of std::runtime_error, widely observed in our code, and encouraged in large part by the std::string constructor.
On the other side, we have some theoretical concerns about a never-observed case based on a incredibly contrived ad-hoc example.
Which one is really “error prone”?
> and
> what sort of usage those classes can support and what sort of errors
> you need to be careful of.
As written above, clearly not.
> If you introduce a new class that mimics a standard library class
It does not mimic it, otherwise we would not have this argument. It does not mimic it on purpose.
> but causes a segfault when used in the same manner as the standard class,
when used in a completely contrived and blatantly bogus example…
while not causing segfaults in another scenario that differs from the previous only in the fact that it reverses the tables, and which you conveniently dismissed for no reason…
> that new class is likely to be misused and is therefore "error-prone”.
Well, practically every step of the “proof” does not stand logically. So the conclusion does not either.
And as I already pointed out above, the actual errors are in the code today. So we have an actual proof by example of std::runtime_error being 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.
It does matter if it’s better (see below). And it does not matter if it’s different, because it’s not intended to replace or “mimic” the original, so the more differences the better.
>
>
>>
>> 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.
It is very much. It shows just how contrived and artificial your example is.
>
>
>>
>> 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.
That does not make the interface the same. It only shows you down-casted when you did not need to, which means the down-cast was only there to try and make a point.
>
>>
>> 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.
This would be error prone. See current use of std::runtime_error as an example.
> I'm not arguing that you need to add a std::string constructor.
If I were to decide that I need a std::string somewhere, I would certainly not reinvent my own. But I don’t need a std::string.
> 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).
If the caller passes me anything bu a compile-time localizable english string constant, the caller is at fault. Why encourage the fault?
Do you have something against requiring string constants in English for our error messages?
Is it so difficult to understand that if I have:
WriteError(“can’t write”, “header”, filename)
WriteError(“can’t write”, “boson”, filename)
I can localize all the combinations “can’t write header”, “can’t write boson” and ignore the filename in the localization process, whereas as soon as I have:
WriteError(“can’t write header” + filename)
it suddenly becomes impossible to do localization?
What is so hard for you to understand in this? I’m getting tired of explaining it.
Now, if I want to “strongly suggest” to the callers that they don’t do the second one, is there a better way to do that than to request the input to be a const char *? I DO NOT WANT A DYNAMIC STRING HERE. Period.
>
>>
>>> 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.
>>
>>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list