Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

2018-02-20 Thread Christophe de Dinechin 


> 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 ) {
// 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
>>>

Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

2018-02-20 Thread Christophe de Dinechin 


> 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 ) {
>>> // 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

Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

2018-02-20 Thread Lukáš Hrázký 
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 ) {
> > >   // 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

Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

2018-02-20 Thread Christophe de Dinechin 


> 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 ) {
>>  // 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 :)

Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

2018-02-20 Thread Lukáš Hrázký 
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 ) {
>   // 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,

Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

2018-02-20 Thread Christophe de Dinechin 


> 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 ) {
// 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

2018-02-19 Thread Frediano Ziglio 
> 
> 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

2018-02-19 Thread Lukáš Hrázký 
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