> On 9 Nov 2017, at 15:39, Frediano Ziglio <fzig...@redhat.com> wrote:
>
>>
>> Christophe Fergeau writes:
>>
>>> On Wed, Nov 08, 2017 at 03:02:35PM +0000, Frediano Ziglio wrote:
>>>> This better integrate with exceptions.
>>>> Also don't leak resources using a return in the middle of the
>>>> code.
>>>>
>>>> Signed-off-by: Frediano Ziglio <fzig...@redhat.com>
>>>> ---
>>>> src/Makefile.am | 1 +
>>>> src/defer.hpp | 16 ++++++++++++++++
>>>> src/spice-streaming-agent.cpp | 16 ++++++++--------
>>>> 3 files changed, 25 insertions(+), 8 deletions(-)
>>>> create mode 100644 src/defer.hpp
>>>>
>>>> diff --git a/src/Makefile.am b/src/Makefile.am
>>>> index 8d5c5bd..ec1969a 100644
>>>> --- a/src/Makefile.am
>>>> +++ b/src/Makefile.am
>>>> @@ -56,4 +56,5 @@ spice_streaming_agent_SOURCES = \
>>>> mjpeg-fallback.cpp \
>>>> jpeg.cpp \
>>>> jpeg.hpp \
>>>> + defer.hpp \
>>>> $(NULL)
>>>> diff --git a/src/defer.hpp b/src/defer.hpp
>>>> new file mode 100644
>>>> index 0000000..93931fe
>>>> --- /dev/null
>>>> +++ b/src/defer.hpp
>>>> @@ -0,0 +1,16 @@
>>>> +// see
>>>> https://stackoverflow.com/questions/32432450/what-is-standard-defer-finalizer-implementation-in-c
>>>
>>> Is there any license associated with that code snippet?
>>
>> That code snippet and variants thereof have been floating around for
>> quite a while. Here is a discussion of this topic I found in 2012:
>>
>> http://the-witness.net/news/2012/11/scopeexit-in-c11/
>>
>> In one of the comments, there is something that is really close.
>>
>> If we want to invent our own by fear of licensing issues, I would go for
>> something like this:
>>
>> struct Defer {
>> template <typename Action> struct Cleanup
>> {
>> ~Cleanup() { action(); }
>> Action action;
>> };
>> template <typename Action>
>> friend inline Cleanup<Action> operator >> (Defer, Action action)
>> {
>> return Cleanup<Action> { action };
>> }
>>
>> };
>> #define my_defer auto _cleanup_ = Defer{} >> [&]() -> void
>>
>> Compared to the original, changes are somewhat minor, but I find them useful:
>>
>> 1. There is no "dummy" class. Whatever is returned is a local
>> Defer::Cleanup class. If we ever need to run that code in a debugger, I
>> think it will be more explicit where things are coming from.
>>
>> 2. I tried to make the code slightly more "readable", e.g. by naming
>> things a bit better, and using operator>> which is more visible in the
>> code than operator*.
>>
>> 3. I use friend name injection to make all the definitions "fit" inside
>> Defer.
>>
>
> Yes, more readable than original.
>
>> 4. I avoid the macro drudgery. Downside is you have only one defer per
>> scope. Upside is you have only one defer per scope. I see that as an
>> upside.
>>
>
> I consider a downside, if I have for instance 2 C open calls and want
> to define 2 defers I cannot.
You can. You just need to put a surrounding block to make the lifetime of
each of your cleanups more explicit.
My argument is really whether this is not dangerous. I think it could be
encouraging bad code like:
fd = open(…)
defer { close(fd); };
close(fd);
// Copy-paste from the above, with variations
fd = open(…)
defer { close(fd); }
kaboom();
close(fd);
This code is bad as in: it is broken on the infrequently taken exception path.
Now we have two defer objects, and if something bad happens in kaboom(),
we end up closing fd twice.
See the issue with multiple defer in a single block and why I’d rather avoid it?
>
> But back to the license "issue" (I don't see it considering the license)
> somebody could consider your suggestion inspired by the previous ones
> so subject to OpenStack licensing too.
There is no such thing as a license on ideas. And as I pointed out, that
specific idea did not even originate where you found it. Being “inspired by”
does not make code derivative. Linux is a prime example, being very
strongly inspired by Unix down to system call names, but not derived from it.
You happen to have found a “defer” macro based on the Go syntax. But
if you came from a Java background, finally has been around for a while.
I’ve seen several variants of macro-based cleanup code that did not even
need templates or lambdas, and worked pretty much the same, stuff like that:
#define micro_defer(Code) micro_defer_(Code,__COUNTER__)
#define micro_defer_(Code,N) micro_defer__(Code,N)
#define micro_defer__(Code,N) struct Defer##N { ~Defer##N() { Code } } defer##N
Usage: micro_defer(fclose(fd)); Only syntactic difference is using () instead
of { }. Not a big deal. Much simpler to understand, easier for the compiler to
process. That stuff has been around for so long I have no idea who to give
credit to, assuming it’s not me.
If you were coming from an iOS background, you’d use that kind of thing on
a semi-regular basis. For example, one that emulates the “defer” keyword in
straight
C here: http://fdiv.net/2015/10/08/emulating-defer-c-clang-or-gccblocks
<http://fdiv.net/2015/10/08/emulating-defer-c-clang-or-gccblocks>.
By the way, its macro drudgery uses __COUNTER__ instead of __LINE__…
If we start considering which license we should apply to every snippet of
code explaining a well-known, if clever, C or C++ trick, we are not out of the
woods.
There are tons of ways to achieve the same objective, with minor syntactic
variations.
So quite frankly, let’s call that a clever programming technique, because
that’s what it is. And then let’s rewrite our own keeping in mind that we want
to write production code and not unreadable snippets, and refer to the on-line
discussion(s) as an explanation for how it works.
>
>>
>>>
>>> Christophe
>>>
>>>> +#ifndef defer
>>>> +template <class F> struct deferrer
>>>> +{
>>>> + F f;
>>>> + ~deferrer() { f(); }
>>>> +};
>>>> +struct defer_dummy {};
>>>> +template <class F> inline deferrer<F> operator*(defer_dummy, F f)
>>>> +{
>>>> + return {f};
>>>> +}
>>>> +#define DFRCAT_(LINE) _defer##LINE
>>>> +#define DFRCAT(LINE) DFRCAT_(LINE)
>>>> +#define defer auto DFRCAT(__LINE__) = defer_dummy{} *[&]() -> void
>>>> +#endif
>>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>>> index ed7ddb9..4122eee 100644
>>>> --- a/src/spice-streaming-agent.cpp
>>>> +++ b/src/spice-streaming-agent.cpp
>>>> @@ -33,6 +33,7 @@
>>>>
>>>> #include "hexdump.h"
>>>> #include "concrete-agent.hpp"
>>>> +#include "defer.hpp"
>>>>
>>>> using namespace std;
>>>> using namespace SpiceStreamingAgent;
>>>> @@ -353,12 +354,17 @@ do_capture(const char *streamport, FILE *f_log)
>>>> // TODO was syslog(LOG_ERR, "Failed to open %s: %s\n",
>>>> streamport, strerror(errno));
>>>> throw std::runtime_error("failed to open streaming device");
>>>>
>>>> + defer {
>>>> + close(streamfd);
>>>> + streamfd = -1;
>>>> + };
>>>> +
>>>> unsigned int frame_count = 0;
>>>> while (! quit) {
>>>> while (!quit && !streaming_requested) {
>>>> if (read_command(1) < 0) {
>>>> syslog(LOG_ERR, "FAILED to read command\n");
>>>> - goto done;
>>>> + return;
>>>> }
>>>> }
>>>>
>>>> @@ -409,19 +415,13 @@ do_capture(const char *streamport, FILE *f_log)
>>>> //usleep(1);
>>>> if (read_command(0) < 0) {
>>>> syslog(LOG_ERR, "FAILED to read command\n");
>>>> - goto done;
>>>> + return;
>>>> }
>>>> if (!streaming_requested) {
>>>> capture->Reset();
>>>> }
>>>> }
>>>> }
>>>> -
>>>> -done:
>>>> - if (streamfd >= 0) {
>>>> - close(streamfd);
>>>> - streamfd = -1;
>>>> - }
>>>> }
>>>>
>>>> #define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org <mailto:Spice-devel@lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> <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
