[Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header
Lukáš Hrázký
lhrazky at redhat.com
Tue Feb 20 14:57:55 UTC 2018
On Tue, 2018-02-20 at 15:48 +0100, Christophe de Dinechin wrote:
> > On 20 Feb 2018, at 15:45, Lukáš Hrázký <lhrazky at redhat.com> wrote:
> >
> > 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 :)
>
> You really only need one class, let’s call it ’SpiceError’, and a discriminating enum.
But the exception you suggested has quite a specific constructor
arguments. We'd eventually need to generalize that, and whatnot... :)
> >
> > 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?
>
> I’d say you should get terminate, since you throw an exception during exception handling.
But you are not handling an exception, you are still constructing one.
It hasn't been thrown yet.
> >
> > Lukas
> >
> > > >
> > > > Frediano
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> > >
> >
> > _______________________________________________
> > 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