I realize I had left this one unanswered.
> On 7 Mar 2018, at 15:14, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:
>
> On Wed, 2018-03-07 at 12:34 +0100, Christophe de Dinechin wrote:
>>> On 6 Mar 2018, at 17:47, Jonathon Jongsma <jjongsma@xxxxxxxxxx>
>>> 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.
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel