> On 19 Feb 2018, at 17:47, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>
>>
>> On Mon, 2018-02-19 at 15:52 +0000, Frediano Ziglio wrote:
>>> Allows to reuse it.
>>>
>>> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
>>> ---
>>> 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?
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel