[Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header
Christophe de Dinechin
cdupontd at redhat.com
Tue Feb 20 14:59:59 UTC 2018
> On 20 Feb 2018, at 15:48, Christophe de Dinechin <cdupontd at redhat.com> 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.
The discriminating enum is only if you wish to filter them separately.
>
>>
>> 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.
Big thinko here, it would be bad_alloc, since it would be thrown before the throw, not after.
>
>>
>> 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