> On 20 Feb 2018, at 15:45, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
>
> On Tue, 2018-02-20 at 15:07 +0100, Christophe de Dinechin wrote:
>>> 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?
>
> 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_ :) 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.
>
> Lukas
>
>>>
>>> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel