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.
>
> 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.
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel