[Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

[Lukáš Hrázký]

On Tue, 2018-02-20 at 15:07 +0100, Christophe de Dinechin wrote:
> > On 19 Feb 2018, at 17:47, Frediano Ziglio <fziglio at redhat.com> wrote:
> > 
> > > 
> > > On Mon, 2018-02-19 at 15:52 +0000, Frediano Ziglio wrote:
> > > > Allows to reuse it.
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > ---
> > > > src/Makefile.am        |  1 +
> > > > src/mjpeg-fallback.cpp |  7 +------
> > > > src/utils.hpp          | 18 ++++++++++++++++++
> > > > 3 files changed, 20 insertions(+), 6 deletions(-)
> > > > create mode 100644 src/utils.hpp
> > > > 
> > > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > > index 3717b5c..ba3b1bf 100644
> > > > --- a/src/Makefile.am
> > > > +++ b/src/Makefile.am
> > > > @@ -55,4 +55,5 @@ spice_streaming_agent_SOURCES = \
> > > > 	mjpeg-fallback.hpp \
> > > > 	jpeg.cpp \
> > > > 	jpeg.hpp \
> > > > +	utils.hpp \
> > > > 	$(NULL)
> > > > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > > > index cf704c6..0f31834 100644
> > > > --- a/src/mjpeg-fallback.cpp
> > > > +++ b/src/mjpeg-fallback.cpp
> > > > @@ -6,6 +6,7 @@
> > > > 
> > > > #include <config.h>
> > > > #include "mjpeg-fallback.hpp"
> > > > +#include "utils.hpp"
> > > > 
> > > > #include <cstring>
> > > > #include <exception>
> > > > @@ -19,12 +20,6 @@
> > > > 
> > > > using namespace spice::streaming_agent;
> > > > 
> > > > -#define ERROR(args) do { \
> > > > -    std::ostringstream _s; \
> > > > -    _s << args; \
> > > > -    throw std::runtime_error(_s.str()); \
> > > > -} while(0)
> > > > -
> > > > static inline uint64_t get_time()
> > > > {
> > > >     timespec now;
> > > > diff --git a/src/utils.hpp b/src/utils.hpp
> > > > new file mode 100644
> > > > index 0000000..1e43eff
> > > > --- /dev/null
> > > > +++ b/src/utils.hpp
> > > > @@ -0,0 +1,18 @@
> > > > +/* Miscellaneous utilities
> > > > + *
> > > > + * \copyright
> > > > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > > > + */
> > > > +#ifndef SPICE_STREAMING_AGENT_UTILS_HPP
> > > > +#define SPICE_STREAMING_AGENT_UTILS_HPP
> > > > +
> > > > +#include <stdexcept>
> > > > +#include <sstream>
> > > > +
> > > > +#define ERROR(args) do { \
> > > > +    std::ostringstream _s; \
> > > > +    _s << args; \
> > > > +    throw std::runtime_error(_s.str()); \
> > > > +} while(0)
> > > > +
> > > > +#endif // SPICE_STREAMING_AGENT_UTILS_HPP
> > > 
> > > Can we please not do this :) It isn't such a chore to throw the
> > > exceptions directly. This adds a level of indirection (code-wise) and
> > > macros are (personal opinion alert) best avoided in C++ unless you
> > > absolutely have to...
> > > 
> > > Lukas
> > > 
> > 
> > Yes, I'm taking to much shortcuts. I was considering a format library like
> > https://github.com/fmtlib/fmt. Did you even used a format library?

Forgot to reply... I haven't used it, and as Christophe, I also
probably wouldn't, though maybe for different reasons :)

> No, I would not do that.
> 
> When you are about to throw an exception, you should probably avoid anything that may fail in a different way, e.g. run out of memory. This is the reason std::runtime_error::what returns a const char * and not a string.
> 
> If you want to do some formatting, may I suggest deferred formatting, which addresses that problem. In other words, instead of:
> 
> 	throw std::runtime_error(std::string(“Flobnic error “) + std::to_string(error_id) + “ when doing “ + operation”);
> 
> consider:
> 
> 	struct flobnic_error : std::runtime_error
> 	{
> 		flobnic_error(const char *message, unsigned error_id, const char *operation)
> 			: std::runtime_error(message), error_id(error_id), operation(operation) {}
> 		// Optional
> 		const char *what() { /* format here, but risky */ }
> 		unsigned error_id;
> 		const char *operation;
> 	}
> 
> and then whoever catches can then properly filter errors:
> 
> 	catch(flobnic_error &f) {
> 		// Emit message here that can use error_id and operation
> 		// If you defined your own “what”, you may use it here
> 		// (and need to decide where the formatted “what” was built
> 		// i.e. whether you need to free it)
> 	}
> 
> It’s a little more work, but it:
> - Passes more information around (both exception type and the original arguments
> - Avoids additional runtime errors that might arise when formatting before throwing.
> 
> 
> Does that make sense?

I don't think this is practical... For some cases, it does make sense
to create exception classes, but in general, we have some many
different errors throughout the code that creating exceptions for each
of them indeed doesn't make any sense :)

How do you propose in general to handle these arbitrary errors, for
which we are throwing runtime_errors now? You may have noticed I've
been replacing some old error handling with just what you described :)
I've also never seen a problem with it, used it a _lot_ :) It would
obviously manifest probably only when running out of memory... In that
case, you'd get some random bad_alloc exception one way or the other?

Lukas

> > 
> > Frediano
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
>