[Spice-devel] [PATCH spice-streaming-agent 2/2] Add a macro to deal with the boilerplate of writing a streaming agent plugin

[Frediano Ziglio]

> 
> > On 23 Nov 2017, at 14:55, Frediano Ziglio <fziglio at redhat.com> wrote:
> > 
> >>> 
> >>> On 23 Nov 2017, at 12:26, Frediano Ziglio <fziglio at redhat.com> wrote:
> >>> 
> >>>> 
> >>>> From: Christophe de Dinechin <dinechin at redhat.com>
> >>>> 
> >>>> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> >>>> ---
> >>>> include/spice-streaming-agent/plugin.hpp | 8 ++++++++
> >>>> 1 file changed, 8 insertions(+)
> >>>> 
> >>>> diff --git a/include/spice-streaming-agent/plugin.hpp
> >>>> b/include/spice-streaming-agent/plugin.hpp
> >>>> index 1a58856..c370eab 100644
> >>>> --- a/include/spice-streaming-agent/plugin.hpp
> >>>> +++ b/include/spice-streaming-agent/plugin.hpp
> >>>> @@ -149,6 +149,14 @@ extern "C" unsigned
> >>>> spice_streaming_agent_plugin_interface_version;
> >>>> */
> >>>> extern "C" SpiceStreamingAgent::PluginInitFunc
> >>>> spice_streaming_agent_plugin_init;
> >>>> 
> >>>> +#define SPICE_STREAMING_AGENT_PLUGIN(agent)
> >>>> \
> >>>> +    __attribute__ ((visibility ("default")))
> >>>> \
> >>>> +    unsigned spice_streaming_agent_plugin_interface_version =
> >>>> \
> >>>> +        SpiceStreamingAgent::PluginInterfaceVersion;
> >>>> \
> >>>> +
> >>>> \
> >>>> +    __attribute__ ((visibility ("default")))
> >>>> \
> >>>> +    bool spice_streaming_agent_plugin_init(SpiceStreamingAgent::Agent*
> >>>> agent)
> >>>> +
> >>>> #endif
> >>>> 
> >>>> #endif // SPICE_STREAMING_AGENT_PLUGIN_HPP
> >>> 
> >>> Surely helps. Some notes:
> >>> - spice_streaming_agent_plugin_interface_version is not const so I assume
> >>> you want to allow to change it;
> >> 
> >> No, it’s that if you make it const, the compiler may optimize it away.
> >> There are two ways I can avoid that:
> >> 1. Use its address
> >> 2. Check that it won’t eliminate it if the __attribute__ above is used.
> >> 
> > 
> > The attribute and the extern "C" should give the compiler quite lot of
> > hints (more the second).
> > 
> >>> - the attribute is GCC syntax but can be changed if needed;
> >> 
> >> This was one reason to put it in a macro.
> >> 
> >> 
> >>> - I know we use that style of indentation for line continuation but I
> >>> honestly prefer to not have that indentation preferring a " \" at the
> >>> end. This as current style:
> >>> - is hard to maintain. Currently is already broken as last like is
> >>>   wrong;
> >> 
> >> It’s actually the other way round with Emacs, which auto-indents
> >> trailing \ (also has a C-c C-\ command (c-backslash-region).
> >> 
> >> The last line is just too long, I will split it.
> >> 
> >>> - it make copy&paste harder unless you indent always at a given
> >>>   standard column;
> >> 
> >> Again, the problem goes the opposite way if you use Emacs.
> >> 
> >>> - make email patch potentially hard to read.
> >> 
> >> Why? Are you using a proportional font when reading patch emails?
> >> 
> > 
> > If lines are long they'll all split in 2 lines.
> > Also if code needs to be indented again there will be a lot of lines
> > of difference just to change indentation forcing people to apply the
> > patch and use some options to ignore spaces which will potentially can
> > hide other unwanted space changes.
> 
> I don’t see how any of this is specific to trailing backslashes.
> You could say the same thing for reindenting code because you
> added an ‘if’.
> 

Consider with indentation

#define XXX      \
    something A  \
    something BB

adding a longer line:

#define XXX       \
    something A   \
    something BB  \
    something CCC 

Now consider without indentation

#define XXX \
    something A \
    something BB

adding a longer line:

#define XXX \
    something A \
    something BB \
    something CCC

The diff (and the patch email) is smaller without indentation.
Yes, an "if" would require some reindentation but having indentation
on left and right increase a lot the probability you need it.

> > 
> >> In any case, that looks like a personal preference, and it goes the
> >> opposite way for me for a variety of reasons:
> >> - Your reasons backwards (when using Emacs to write and read patches)
> >> - Emacs Is Always Right™
> >> - Having \ at a known location helps me “put it out of the way” while
> >> reading.
> >> 
> >> 
> >> Cheers,
> >> Christophe
> >> 
> > 
> > Half of your reasoning are based on the editor used, if I remove
> > Emacs from them basically you agree with me :-)
> 
> All of our respective reasonings are based on personal preferences.
> Those, in turn, derive from the tools we use. I am not trying to make
> them anything else than personal preferences. I was only explaining
> that my editor choice happens to turn 100% of your arguments backwards.
> 
> But no, I don’t agree that code with misaligned trailing \ looks better
> or is easier to read than aligned code. It makes it hard to read for me,
> and I see no real reason to write code I find hard to read :-)
> 

Yes, we agree is style. Personally I find not indentation as readable
as the other style.

Frediano