[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 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.

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

Frediano