Re: [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  wrote:
> > 
> >>> 
> >>> On 23 Nov 2017, at 12:26, Frediano Ziglio  wrote:
> >>> 
>  
>  From: Christophe de Dinechin 
>  
>  Signed-off-by: Christophe de Dinechin 
>  ---
>  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 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

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

[Christophe de Dinechin]

> On 23 Nov 2017, at 14:55, Frediano Ziglio  wrote:
> 
>>> 
>>> On 23 Nov 2017, at 12:26, Frediano Ziglio  wrote:
>>> 
 
 From: Christophe de Dinechin 
 
 Signed-off-by: Christophe de Dinechin 
 ---
 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 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’.

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

> 
> 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/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  wrote:
> > 
> >> 
> >> From: Christophe de Dinechin 
> >> 
> >> Signed-off-by: Christophe de Dinechin 
> >> ---
> >> 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 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
___
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/2] Add a macro to deal with the boilerplate of writing a streaming agent plugin

[Christophe de Dinechin]

> On 23 Nov 2017, at 12:26, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> 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 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 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?

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

___
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/2] Add a macro to deal with the boilerplate of writing a streaming agent plugin

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  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;
- the attribute is GCC syntax but can be changed if needed;
- 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 make copy harder unless you indent always at a given
standard column;
  - make email patch potentially hard to read.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel