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

[Christophe de Dinechin]

> 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?

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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel