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

[Jonathon Jongsma]

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

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