Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header
> On 20 Feb 2018, at 15:57, Lukáš Hrázký wrote: > > On Tue, 2018-02-20 at 15:48 +0100, Christophe de Dinechin wrote: >>> On 20 Feb 2018, at 15:45, Lukáš Hrázký wrote: >>> >>> On Tue, 2018-02-20 at 15:07 +0100, Christophe de Dinechin wrote: > On 19 Feb 2018, at 17:47, Frediano Ziglio wrote: > >> >> On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote: >>> Allows to reuse it. >>> >>> Signed-off-by: Frediano Ziglio >>> --- >>> 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 >>> #include "mjpeg-fallback.hpp" >>> +#include "utils.hpp" >>> >>> #include >>> #include >>> @@ -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 000..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 >>> +#include >>> + >>> +#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 the
Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header
> On 20 Feb 2018, at 15:48, Christophe de Dinechin wrote: > > > >> On 20 Feb 2018, at 15:45, Lukáš Hrázký wrote: >> >> On Tue, 2018-02-20 at 15:07 +0100, Christophe de Dinechin wrote: On 19 Feb 2018, at 17:47, Frediano Ziglio wrote: > > On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote: >> Allows to reuse it. >> >> Signed-off-by: Frediano Ziglio >> --- >> 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 >> #include "mjpeg-fallback.hpp" >> +#include "utils.hpp" >> >> #include >> #include >> @@ -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 000..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 >> +#include >> + >> +#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. The discriminating enum is only if you wish to filter them separately. > >
Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header
On Tue, 2018-02-20 at 15:48 +0100, Christophe de Dinechin wrote: > > On 20 Feb 2018, at 15:45, Lukáš Hrázký wrote: > > > > On Tue, 2018-02-20 at 15:07 +0100, Christophe de Dinechin wrote: > > > > On 19 Feb 2018, at 17:47, Frediano Ziglio wrote: > > > > > > > > > > > > > > On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote: > > > > > > Allows to reuse it. > > > > > > > > > > > > Signed-off-by: Frediano Ziglio > > > > > > --- > > > > > > 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 > > > > > > #include "mjpeg-fallback.hpp" > > > > > > +#include "utils.hpp" > > > > > > > > > > > > #include > > > > > > #include > > > > > > @@ -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 000..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 > > > > > > +#include > > > > > > + > > > > > > +#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 throw
Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header
> On 20 Feb 2018, at 15:45, Lukáš Hrázký wrote: > > On Tue, 2018-02-20 at 15:07 +0100, Christophe de Dinechin wrote: >>> On 19 Feb 2018, at 17:47, Frediano Ziglio wrote: >>> On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote: > Allows to reuse it. > > Signed-off-by: Frediano Ziglio > --- > 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 > #include "mjpeg-fallback.hpp" > +#include "utils.hpp" > > #include > #include > @@ -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 000..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 > +#include > + > +#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. > > 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_ :
Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header
On Tue, 2018-02-20 at 15:07 +0100, Christophe de Dinechin wrote: > > On 19 Feb 2018, at 17:47, Frediano Ziglio wrote: > > > > > > > > On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote: > > > > Allows to reuse it. > > > > > > > > Signed-off-by: Frediano Ziglio > > > > --- > > > > 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 > > > > #include "mjpeg-fallback.hpp" > > > > +#include "utils.hpp" > > > > > > > > #include > > > > #include > > > > @@ -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 000..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 > > > > +#include > > > > + > > > > +#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 m
Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header
> On 19 Feb 2018, at 17:47, Frediano Ziglio wrote: > >> >> On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote: >>> Allows to reuse it. >>> >>> Signed-off-by: Frediano Ziglio >>> --- >>> 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 >>> #include "mjpeg-fallback.hpp" >>> +#include "utils.hpp" >>> >>> #include >>> #include >>> @@ -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 000..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 >>> +#include >>> + >>> +#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? 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? > > Frediano > ___ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header
> > On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote: > > Allows to reuse it. > > > > Signed-off-by: Frediano Ziglio > > --- > > 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 > > #include "mjpeg-fallback.hpp" > > +#include "utils.hpp" > > > > #include > > #include > > @@ -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 000..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 > > +#include > > + > > +#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? Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header
On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote: > Allows to reuse it. > > Signed-off-by: Frediano Ziglio > --- > 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 > #include "mjpeg-fallback.hpp" > +#include "utils.hpp" > > #include > #include > @@ -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 000..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 > +#include > + > +#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 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel