Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

2018-02-15 Thread Christophe Fergeau 
On Thu, Feb 15, 2018 at 04:04:57PM +0100, Christophe de Dinechin wrote:
> 
>   This style guide only indicates what we aim to achieve. It does not 
> necessarily reflect the current state of the code.
> 
> What about adding:
> 
>   Consistency matters. It may be preferable to ignore a style 
> recommendation if it helps keeping the code style consistent.

Sounds good to me.

> 
> And in the header part that you were adjusting, I’d be happy if you simply 
> added:
> 
> +C++ headers guards should use // comments.
> +The server consistently use /* */ comments for header guards

I can add the last line if you want, but in my opinion, your
'consistency' addition is enough for that.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

2018-02-15 Thread Christophe de Dinechin 


> On 15 Feb 2018, at 15:55, Christophe Fergeau  wrote:
> 
> On Thu, Feb 15, 2018 at 03:25:23PM +0100, Christophe de Dinechin wrote:
>> 
>> 
>>> On 15 Feb 2018, at 13:41, Christophe Fergeau  wrote:
>>> 
>>> On Thu, Feb 15, 2018 at 11:55:44AM +0100, Christophe de Dinechin wrote:
 Now, Christophe’s arguments are that
 
 1) we should not write guidelines that are inconsistent with existing code.
 2) this is in the server codebase, so we should server rules
 
 Problem is with 2, really.
 
 We started updating the guidelines because we wanted to talk about C++
 style in the streaming agent, not the server.
 I updated the server guidelines, because that’s historically where the
 style guide has been, and the only place where SPICE has one.
 
 For now, I’d vote for stating that the server guidelines apply to all
 of the SPICE code. If we decide that means we should move them
 elsewhere, that’s fine with me.
 
 If that idea is accepted, then Christophe (2) no longer hold, and we
 can explicitly state that we accept both // and /* for all comments,
 including that one.
>>> 
>>> My patch was changing the example from using // to using /*,
>> 
>> That part I’m OK with.
>> 
>>> and was
>>> adding a note explicitly saying // was acceptable too.
>> 
>> That may have been your intent, but the way you wrote it was:
>> 
>> "C++ headers would use C++ comments."
>> 
>> This suggests that you can’t use // in C headers. Is that something we 
>> really want to enforce?
>> 
>> Again, I’d much prefer that we write somewhere that // or /* comments are 
>> both OK (not specifically for headers guards)
> 
> All I care about is that things stay mostly consistent within a given
> project. I don't want someone to come and say "oh but it's written in
> the coding style that I can use #endif // MY_MODULE_H!!" when the rest
> of the codebase is not using that.

Agreed. Worth adding, maybe as a general remark.

In my current staging area, I have near the beginning:

This style guide only indicates what we aim to achieve. It does not 
necessarily reflect the current state of the code.

What about adding:

Consistency matters. It may be preferable to ignore a style 
recommendation if it helps keeping the code style consistent.

And in the header part that you were adjusting, I’d be happy if you simply 
added:

+C++ headers guards should use // comments.
+The server consistently use /* */ comments for header guards


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

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

2018-02-15 Thread Christophe Fergeau 
On Thu, Feb 15, 2018 at 03:25:23PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 15 Feb 2018, at 13:41, Christophe Fergeau  wrote:
> > 
> > On Thu, Feb 15, 2018 at 11:55:44AM +0100, Christophe de Dinechin wrote:
> >> Now, Christophe’s arguments are that
> >> 
> >> 1) we should not write guidelines that are inconsistent with existing code.
> >> 2) this is in the server codebase, so we should server rules
> >> 
> >> Problem is with 2, really.
> >> 
> >> We started updating the guidelines because we wanted to talk about C++
> >> style in the streaming agent, not the server.
> >> I updated the server guidelines, because that’s historically where the
> >> style guide has been, and the only place where SPICE has one.
> >> 
> >> For now, I’d vote for stating that the server guidelines apply to all
> >> of the SPICE code. If we decide that means we should move them
> >> elsewhere, that’s fine with me.
> >> 
> >> If that idea is accepted, then Christophe (2) no longer hold, and we
> >> can explicitly state that we accept both // and /* for all comments,
> >> including that one.
> > 
> > My patch was changing the example from using // to using /*,
> 
> That part I’m OK with.
> 
> > and was
> > adding a note explicitly saying // was acceptable too.
> 
> That may have been your intent, but the way you wrote it was:
> 
> "C++ headers would use C++ comments."
> 
> This suggests that you can’t use // in C headers. Is that something we really 
> want to enforce?
> 
> Again, I’d much prefer that we write somewhere that // or /* comments are 
> both OK (not specifically for headers guards)

All I care about is that things stay mostly consistent within a given
project. I don't want someone to come and say "oh but it's written in
the coding style that I can use #endif // MY_MODULE_H!!" when the rest
of the codebase is not using that.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

2018-02-15 Thread Christophe de Dinechin 


> On 15 Feb 2018, at 13:41, Christophe Fergeau  wrote:
> 
> On Thu, Feb 15, 2018 at 11:55:44AM +0100, Christophe de Dinechin wrote:
>> Now, Christophe’s arguments are that
>> 
>> 1) we should not write guidelines that are inconsistent with existing code.
>> 2) this is in the server codebase, so we should server rules
>> 
>> Problem is with 2, really.
>> 
>> We started updating the guidelines because we wanted to talk about C++
>> style in the streaming agent, not the server.
>> I updated the server guidelines, because that’s historically where the
>> style guide has been, and the only place where SPICE has one.
>> 
>> For now, I’d vote for stating that the server guidelines apply to all
>> of the SPICE code. If we decide that means we should move them
>> elsewhere, that’s fine with me.
>> 
>> If that idea is accepted, then Christophe (2) no longer hold, and we
>> can explicitly state that we accept both // and /* for all comments,
>> including that one.
> 
> My patch was changing the example from using // to using /*,

That part I’m OK with.

> and was
> adding a note explicitly saying // was acceptable too.

That may have been your intent, but the way you wrote it was:

"C++ headers would use C++ comments."

This suggests that you can’t use // in C headers. Is that something we really 
want to enforce?

Again, I’d much prefer that we write somewhere that // or /* comments are both 
OK (not specifically for headers guards)


> 
> Christophe
> ___
> 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] [spice-server] style: Slight tweak to the header guard section

2018-02-15 Thread Christophe Fergeau 
On Thu, Feb 15, 2018 at 11:55:44AM +0100, Christophe de Dinechin wrote:
> Now, Christophe’s arguments are that
> 
> 1) we should not write guidelines that are inconsistent with existing code.
> 2) this is in the server codebase, so we should server rules
> 
> Problem is with 2, really.
> 
> We started updating the guidelines because we wanted to talk about C++
> style in the streaming agent, not the server.
> I updated the server guidelines, because that’s historically where the
> style guide has been, and the only place where SPICE has one.
> 
> For now, I’d vote for stating that the server guidelines apply to all
> of the SPICE code. If we decide that means we should move them
> elsewhere, that’s fine with me.
> 
> If that idea is accepted, then Christophe (2) no longer hold, and we
> can explicitly state that we accept both // and /* for all comments,
> including that one.

My patch was changing the example from using // to using /*, and was
adding a note explicitly saying // was acceptable too.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

2018-02-15 Thread Frediano Ziglio 
> 
> > On 15 Feb 2018, at 11:43, Frediano Ziglio  wrote:
> > 
> >> 
> >> On Thu, Feb 15, 2018 at 10:56:49AM +0100, Lukáš Hrázký wrote:
> >>> On Wed, 2018-02-14 at 22:43 +0100, Christophe de Dinechin wrote:
> > On 14 Feb 2018, at 17:29, Christophe Fergeau 
> > wrote:
> > 
> > This changes the suggested style to what is currently used in
> > spice-server codebase. This also removes a few sentences which
> > are not really related to how one should format their header guards.
> > 
> > Signed-off-by: Christophe Fergeau 
> > ---
> > docs/spice_style.txt | 6 ++
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> > index c8a4cff66..bc18b1d9e 100644
> > --- a/docs/spice_style.txt
> > +++ b/docs/spice_style.txt
> > @@ -365,12 +365,10 @@ Headers should be protected against multiple
> > inclusion using a macro that contai
> > 
> > ...
> > 
> > -#endif // MY_MODULE_H
> > +#endif /* MY_MODULE_H */
>  
>  I had first written it with C style, Frediano suggested C++ style.Both
>  are OK. Currently, we have 44 of one and 208 of the other, so the
>  existing code base does not imply one or the other.
> >>> 
> >>> FWIW, I see no reason to use /* */ when // is simpler.
> >> 
> >> I don't feel strongly either way as long as it's consistent, and in
> >> spice-server case:
> >> 
> >> $ for f in spice/server/*.h; do tail -1 $f; done
> >> #endif /* AGENT_MSG_FILTER_H_ */
> >> #endif /* CACHE_ITEM_H_ */
> >> #endif /* CHAR_DEVICE_H_ */
> >> #endif /* COMMON_GRAPHICS_CHANNEL_H_ */
> >> #endif /* CURSOR_CHANNEL_CLIENT_H_ */
> >> #endif /* CURSOR_CHANNEL_H_ */
> >> #endif /* DCC_H_ */
> >> #endif /* DCC_PRIVATE_H_ */
> >> #endif /* DEMARSHALLERS_H_ */
> >> #endif /* DISPATCHER_H_ */
> >> #endif /* DISPLAY_CHANNEL_H_ */
> >> #endif /* DISPLAY_CHANNEL_PRIVATE_H_ */
> >> #endif /* DISPLAY_LIMITS_H_ */
> >> #endif /* GLIB_COMPAT_H_ */
> >> #endif /* GLZ_ENCODER_DICT_H_ */
> >> #endif /* GLZ_ENCODER_H_ */
> >> #endif /* GLZ_ENCODER_PRIV_H_ */
> >> #endif /* IMAGE_CACHE_H_ */
> >> #endif /* IMAGE_ENCODERS_H_ */
> >> #endif /* INPUTS_CHANNEL_CLIENT_H_ */
> >> #endif /* INPUTS_CHANNEL_H_ */
> >> #endif /* JPEG_ENCODER_H_ */
> >> #endif /* LZ4_ENCODER_H_ */
> >> #endif /* MAIN_CHANNEL_CLIENT_H_ */
> >> #endif /* MAIN_CHANNEL_H_ */
> >> #endif /* MAIN_DISPATCHER_H_ */
> >> #endif /* MEMSLOT_H_ */
> >> #endif /* MIGRATION_PROTOCOL_H_ */
> >> #endif /* RED_NET_UTILS_H_ */
> >> #endif /* PIXMAP_CACHE_H_ */
> >> #endif /* RED_CHANNEL_CAPABILITIES_H_ */
> >> #endif /* RED_CHANNEL_CLIENT_H_ */
> >> #endif /* RED_CHANNEL_H_ */
> >> #endif /* RED_CLIENT_H_ */
> >> #endif /* RED_COMMON_H_ */
> >> #endif /* RED_PARSE_QXL_H_ */
> >> 
> >> #endif /* RED_PIPE_ITEM_H_ */
> >> #endif /* RED_QXL_H_ */
> >> #endif /* RED_RECORD_QXL_H_ */
> >> #endif /* REDS_H_ */
> >> #endif /* REDS_PRIVATE_H_ */
> >> #endif /* RED_STREAM_H_ */
> >> #endif /* RED_WORKER_H_ */
> >> #endif /* SMARTCARD_CHANNEL_CLIENT_H_ */
> >> #endif /* SMART_CARD_H_ */
> >> #endif /* SOUND_H_ */
> >> #endif /* SPICE_AUDIO_H_ */
> >> #endif /* SPICE_BITMAP_UTILS_H_ */
> >> #endif /* SPICE_CHAR_H_ */
> >> #endif /* SPICE_CORE_H_ */
> >> #endif /* SPICE_EXPERIMENTAL_H_ */
> >> #endif /* SPICE_H_ */
> >> #endif /* SPICE_INPUT_H_ */
> >> #endif /* SPICE_MIGRATION_H_ */
> >> #endif /* SPICE_QXL_H_ */
> >> #endif /* SPICE_REPLAY_H_ */
> >> 
> >> /*** END file-tail ***/
> >> #endif /* SPICE_SERVER_H_ */
> >> #endif /* SPICE_VERSION_H_ */
> >> #endif /* STAT_FILE_H_ */
> >> #endif /* STAT_H_ */
> >> #endif /* STREAM_CHANNEL_H_ */
> >> #endif /* TREE_H_ */
> >> #endif /* UTILS_H_ */
> >> #endif /* VIDEO_ENCODER_H_ */
> >> #endif /* VIDEO_STREAM_H_ */
> >> #endif /* ZLIB_ENCODER_H_ */
> >> 
> > 
> > Maybe I helped creating this confusion.
> > 
> > I was looking at the example of the session talking about header guard
> > and my though was "let's hope this final comment style is not taken
> > literally, the section is about having the guards".
> > Personally C style, C++ style or a missing comment (I think in 95% of
> > all headers the last #endif is the close of the guard!) is not that
> > important.
> > From the current style point of view (and taking into account that
> > this document is in spice-server) yes, the suggested style for closure
> > would be better to have a C style comment with the guard name.
> > Sorry for the confusion.
> > 
> > Maybe just a comment stating the end comment style is just an advice?
> 
> I don’t think we want this to be just an advice. The problem Christophe
> points out is with existing code.
> I think that if we started today, we could all agree on //
> 
> Now, Christophe’s arguments are that
> 
> 1) we should not write guidelines that are inconsistent with existing code.
> 2) this is in the server codebase, so we should server

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

2018-02-15 Thread Christophe de Dinechin 


> On 15 Feb 2018, at 11:43, Frediano Ziglio  wrote:
> 
>> 
>> On Thu, Feb 15, 2018 at 10:56:49AM +0100, Lukáš Hrázký wrote:
>>> On Wed, 2018-02-14 at 22:43 +0100, Christophe de Dinechin wrote:
> On 14 Feb 2018, at 17:29, Christophe Fergeau 
> wrote:
> 
> This changes the suggested style to what is currently used in
> spice-server codebase. This also removes a few sentences which
> are not really related to how one should format their header guards.
> 
> Signed-off-by: Christophe Fergeau 
> ---
> docs/spice_style.txt | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index c8a4cff66..bc18b1d9e 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -365,12 +365,10 @@ Headers should be protected against multiple
> inclusion using a macro that contai
> 
> ...
> 
> -#endif // MY_MODULE_H
> +#endif /* MY_MODULE_H */
 
 I had first written it with C style, Frediano suggested C++ style.Both
 are OK. Currently, we have 44 of one and 208 of the other, so the
 existing code base does not imply one or the other.
>>> 
>>> FWIW, I see no reason to use /* */ when // is simpler.
>> 
>> I don't feel strongly either way as long as it's consistent, and in
>> spice-server case:
>> 
>> $ for f in spice/server/*.h; do tail -1 $f; done
>> #endif /* AGENT_MSG_FILTER_H_ */
>> #endif /* CACHE_ITEM_H_ */
>> #endif /* CHAR_DEVICE_H_ */
>> #endif /* COMMON_GRAPHICS_CHANNEL_H_ */
>> #endif /* CURSOR_CHANNEL_CLIENT_H_ */
>> #endif /* CURSOR_CHANNEL_H_ */
>> #endif /* DCC_H_ */
>> #endif /* DCC_PRIVATE_H_ */
>> #endif /* DEMARSHALLERS_H_ */
>> #endif /* DISPATCHER_H_ */
>> #endif /* DISPLAY_CHANNEL_H_ */
>> #endif /* DISPLAY_CHANNEL_PRIVATE_H_ */
>> #endif /* DISPLAY_LIMITS_H_ */
>> #endif /* GLIB_COMPAT_H_ */
>> #endif /* GLZ_ENCODER_DICT_H_ */
>> #endif /* GLZ_ENCODER_H_ */
>> #endif /* GLZ_ENCODER_PRIV_H_ */
>> #endif /* IMAGE_CACHE_H_ */
>> #endif /* IMAGE_ENCODERS_H_ */
>> #endif /* INPUTS_CHANNEL_CLIENT_H_ */
>> #endif /* INPUTS_CHANNEL_H_ */
>> #endif /* JPEG_ENCODER_H_ */
>> #endif /* LZ4_ENCODER_H_ */
>> #endif /* MAIN_CHANNEL_CLIENT_H_ */
>> #endif /* MAIN_CHANNEL_H_ */
>> #endif /* MAIN_DISPATCHER_H_ */
>> #endif /* MEMSLOT_H_ */
>> #endif /* MIGRATION_PROTOCOL_H_ */
>> #endif /* RED_NET_UTILS_H_ */
>> #endif /* PIXMAP_CACHE_H_ */
>> #endif /* RED_CHANNEL_CAPABILITIES_H_ */
>> #endif /* RED_CHANNEL_CLIENT_H_ */
>> #endif /* RED_CHANNEL_H_ */
>> #endif /* RED_CLIENT_H_ */
>> #endif /* RED_COMMON_H_ */
>> #endif /* RED_PARSE_QXL_H_ */
>> 
>> #endif /* RED_PIPE_ITEM_H_ */
>> #endif /* RED_QXL_H_ */
>> #endif /* RED_RECORD_QXL_H_ */
>> #endif /* REDS_H_ */
>> #endif /* REDS_PRIVATE_H_ */
>> #endif /* RED_STREAM_H_ */
>> #endif /* RED_WORKER_H_ */
>> #endif /* SMARTCARD_CHANNEL_CLIENT_H_ */
>> #endif /* SMART_CARD_H_ */
>> #endif /* SOUND_H_ */
>> #endif /* SPICE_AUDIO_H_ */
>> #endif /* SPICE_BITMAP_UTILS_H_ */
>> #endif /* SPICE_CHAR_H_ */
>> #endif /* SPICE_CORE_H_ */
>> #endif /* SPICE_EXPERIMENTAL_H_ */
>> #endif /* SPICE_H_ */
>> #endif /* SPICE_INPUT_H_ */
>> #endif /* SPICE_MIGRATION_H_ */
>> #endif /* SPICE_QXL_H_ */
>> #endif /* SPICE_REPLAY_H_ */
>> 
>> /*** END file-tail ***/
>> #endif /* SPICE_SERVER_H_ */
>> #endif /* SPICE_VERSION_H_ */
>> #endif /* STAT_FILE_H_ */
>> #endif /* STAT_H_ */
>> #endif /* STREAM_CHANNEL_H_ */
>> #endif /* TREE_H_ */
>> #endif /* UTILS_H_ */
>> #endif /* VIDEO_ENCODER_H_ */
>> #endif /* VIDEO_STREAM_H_ */
>> #endif /* ZLIB_ENCODER_H_ */
>> 
> 
> Maybe I helped creating this confusion.
> 
> I was looking at the example of the session talking about header guard
> and my though was "let's hope this final comment style is not taken
> literally, the section is about having the guards".
> Personally C style, C++ style or a missing comment (I think in 95% of
> all headers the last #endif is the close of the guard!) is not that
> important.
> From the current style point of view (and taking into account that
> this document is in spice-server) yes, the suggested style for closure
> would be better to have a C style comment with the guard name.
> Sorry for the confusion.
> 
> Maybe just a comment stating the end comment style is just an advice?

I don’t think we want this to be just an advice. The problem Christophe points 
out is with existing code.
I think that if we started today, we could all agree on //

Now, Christophe’s arguments are that

1) we should not write guidelines that are inconsistent with existing code.
2) this is in the server codebase, so we should server rules

Problem is with 2, really.

We started updating the guidelines because we wanted to talk about C++ style in 
the streaming agent, not the server.
I updated the server guidelines, because that’s historically where the style 
guide has been, and the only place

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

2018-02-15 Thread Frediano Ziglio 
> 
> On Thu, Feb 15, 2018 at 10:56:49AM +0100, Lukáš Hrázký wrote:
> > On Wed, 2018-02-14 at 22:43 +0100, Christophe de Dinechin wrote:
> > > > On 14 Feb 2018, at 17:29, Christophe Fergeau 
> > > > wrote:
> > > > 
> > > > This changes the suggested style to what is currently used in
> > > > spice-server codebase. This also removes a few sentences which
> > > > are not really related to how one should format their header guards.
> > > > 
> > > > Signed-off-by: Christophe Fergeau 
> > > > ---
> > > > docs/spice_style.txt | 6 ++
> > > > 1 file changed, 2 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> > > > index c8a4cff66..bc18b1d9e 100644
> > > > --- a/docs/spice_style.txt
> > > > +++ b/docs/spice_style.txt
> > > > @@ -365,12 +365,10 @@ Headers should be protected against multiple
> > > > inclusion using a macro that contai
> > > > 
> > > > ...
> > > > 
> > > > -#endif // MY_MODULE_H
> > > > +#endif /* MY_MODULE_H */
> > > 
> > > I had first written it with C style, Frediano suggested C++ style.Both
> > > are OK. Currently, we have 44 of one and 208 of the other, so the
> > > existing code base does not imply one or the other.
> > 
> > FWIW, I see no reason to use /* */ when // is simpler.
> 
> I don't feel strongly either way as long as it's consistent, and in
> spice-server case:
> 
> $ for f in spice/server/*.h; do tail -1 $f; done
> #endif /* AGENT_MSG_FILTER_H_ */
> #endif /* CACHE_ITEM_H_ */
> #endif /* CHAR_DEVICE_H_ */
> #endif /* COMMON_GRAPHICS_CHANNEL_H_ */
> #endif /* CURSOR_CHANNEL_CLIENT_H_ */
> #endif /* CURSOR_CHANNEL_H_ */
> #endif /* DCC_H_ */
> #endif /* DCC_PRIVATE_H_ */
> #endif /* DEMARSHALLERS_H_ */
> #endif /* DISPATCHER_H_ */
> #endif /* DISPLAY_CHANNEL_H_ */
> #endif /* DISPLAY_CHANNEL_PRIVATE_H_ */
> #endif /* DISPLAY_LIMITS_H_ */
> #endif /* GLIB_COMPAT_H_ */
> #endif /* GLZ_ENCODER_DICT_H_ */
> #endif /* GLZ_ENCODER_H_ */
> #endif /* GLZ_ENCODER_PRIV_H_ */
> #endif /* IMAGE_CACHE_H_ */
> #endif /* IMAGE_ENCODERS_H_ */
> #endif /* INPUTS_CHANNEL_CLIENT_H_ */
> #endif /* INPUTS_CHANNEL_H_ */
> #endif /* JPEG_ENCODER_H_ */
> #endif /* LZ4_ENCODER_H_ */
> #endif /* MAIN_CHANNEL_CLIENT_H_ */
> #endif /* MAIN_CHANNEL_H_ */
> #endif /* MAIN_DISPATCHER_H_ */
> #endif /* MEMSLOT_H_ */
> #endif /* MIGRATION_PROTOCOL_H_ */
> #endif /* RED_NET_UTILS_H_ */
> #endif /* PIXMAP_CACHE_H_ */
> #endif /* RED_CHANNEL_CAPABILITIES_H_ */
> #endif /* RED_CHANNEL_CLIENT_H_ */
> #endif /* RED_CHANNEL_H_ */
> #endif /* RED_CLIENT_H_ */
> #endif /* RED_COMMON_H_ */
> #endif /* RED_PARSE_QXL_H_ */
> 
> #endif /* RED_PIPE_ITEM_H_ */
> #endif /* RED_QXL_H_ */
> #endif /* RED_RECORD_QXL_H_ */
> #endif /* REDS_H_ */
> #endif /* REDS_PRIVATE_H_ */
> #endif /* RED_STREAM_H_ */
> #endif /* RED_WORKER_H_ */
> #endif /* SMARTCARD_CHANNEL_CLIENT_H_ */
> #endif /* SMART_CARD_H_ */
> #endif /* SOUND_H_ */
> #endif /* SPICE_AUDIO_H_ */
> #endif /* SPICE_BITMAP_UTILS_H_ */
> #endif /* SPICE_CHAR_H_ */
> #endif /* SPICE_CORE_H_ */
> #endif /* SPICE_EXPERIMENTAL_H_ */
> #endif /* SPICE_H_ */
> #endif /* SPICE_INPUT_H_ */
> #endif /* SPICE_MIGRATION_H_ */
> #endif /* SPICE_QXL_H_ */
> #endif /* SPICE_REPLAY_H_ */
> 
> /*** END file-tail ***/
> #endif /* SPICE_SERVER_H_ */
> #endif /* SPICE_VERSION_H_ */
> #endif /* STAT_FILE_H_ */
> #endif /* STAT_H_ */
> #endif /* STREAM_CHANNEL_H_ */
> #endif /* TREE_H_ */
> #endif /* UTILS_H_ */
> #endif /* VIDEO_ENCODER_H_ */
> #endif /* VIDEO_STREAM_H_ */
> #endif /* ZLIB_ENCODER_H_ */
> 

Maybe I helped creating this confusion.

I was looking at the example of the session talking about header guard
and my though was "let's hope this final comment style is not taken
literally, the section is about having the guards".
Personally C style, C++ style or a missing comment (I think in 95% of
all headers the last #endif is the close of the guard!) is not that
important.
From the current style point of view (and taking into account that
this document is in spice-server) yes, the suggested style for closure
would be better to have a C style comment with the guard name.
Sorry for the confusion.

Maybe just a comment stating the end comment style is just an advice?

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

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

2018-02-15 Thread Christophe Fergeau 
On Thu, Feb 15, 2018 at 10:56:49AM +0100, Lukáš Hrázký wrote:
> On Wed, 2018-02-14 at 22:43 +0100, Christophe de Dinechin wrote:
> > > On 14 Feb 2018, at 17:29, Christophe Fergeau  wrote:
> > > 
> > > This changes the suggested style to what is currently used in
> > > spice-server codebase. This also removes a few sentences which
> > > are not really related to how one should format their header guards.
> > > 
> > > Signed-off-by: Christophe Fergeau 
> > > ---
> > > docs/spice_style.txt | 6 ++
> > > 1 file changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> > > index c8a4cff66..bc18b1d9e 100644
> > > --- a/docs/spice_style.txt
> > > +++ b/docs/spice_style.txt
> > > @@ -365,12 +365,10 @@ Headers should be protected against multiple 
> > > inclusion using a macro that contai
> > > 
> > > ...
> > > 
> > > -#endif // MY_MODULE_H
> > > +#endif /* MY_MODULE_H */
> > 
> > I had first written it with C style, Frediano suggested C++ style.Both are 
> > OK. Currently, we have 44 of one and 208 of the other, so the existing code 
> > base does not imply one or the other.
> 
> FWIW, I see no reason to use /* */ when // is simpler.

I don't feel strongly either way as long as it's consistent, and in
spice-server case:

$ for f in spice/server/*.h; do tail -1 $f; done
#endif /* AGENT_MSG_FILTER_H_ */
#endif /* CACHE_ITEM_H_ */
#endif /* CHAR_DEVICE_H_ */
#endif /* COMMON_GRAPHICS_CHANNEL_H_ */
#endif /* CURSOR_CHANNEL_CLIENT_H_ */
#endif /* CURSOR_CHANNEL_H_ */
#endif /* DCC_H_ */
#endif /* DCC_PRIVATE_H_ */
#endif /* DEMARSHALLERS_H_ */
#endif /* DISPATCHER_H_ */
#endif /* DISPLAY_CHANNEL_H_ */
#endif /* DISPLAY_CHANNEL_PRIVATE_H_ */
#endif /* DISPLAY_LIMITS_H_ */
#endif /* GLIB_COMPAT_H_ */
#endif /* GLZ_ENCODER_DICT_H_ */
#endif /* GLZ_ENCODER_H_ */
#endif /* GLZ_ENCODER_PRIV_H_ */
#endif /* IMAGE_CACHE_H_ */
#endif /* IMAGE_ENCODERS_H_ */
#endif /* INPUTS_CHANNEL_CLIENT_H_ */
#endif /* INPUTS_CHANNEL_H_ */
#endif /* JPEG_ENCODER_H_ */
#endif /* LZ4_ENCODER_H_ */
#endif /* MAIN_CHANNEL_CLIENT_H_ */
#endif /* MAIN_CHANNEL_H_ */
#endif /* MAIN_DISPATCHER_H_ */
#endif /* MEMSLOT_H_ */
#endif /* MIGRATION_PROTOCOL_H_ */
#endif /* RED_NET_UTILS_H_ */
#endif /* PIXMAP_CACHE_H_ */
#endif /* RED_CHANNEL_CAPABILITIES_H_ */
#endif /* RED_CHANNEL_CLIENT_H_ */
#endif /* RED_CHANNEL_H_ */
#endif /* RED_CLIENT_H_ */
#endif /* RED_COMMON_H_ */
#endif /* RED_PARSE_QXL_H_ */

#endif /* RED_PIPE_ITEM_H_ */
#endif /* RED_QXL_H_ */
#endif /* RED_RECORD_QXL_H_ */
#endif /* REDS_H_ */
#endif /* REDS_PRIVATE_H_ */
#endif /* RED_STREAM_H_ */
#endif /* RED_WORKER_H_ */
#endif /* SMARTCARD_CHANNEL_CLIENT_H_ */
#endif /* SMART_CARD_H_ */
#endif /* SOUND_H_ */
#endif /* SPICE_AUDIO_H_ */
#endif /* SPICE_BITMAP_UTILS_H_ */
#endif /* SPICE_CHAR_H_ */
#endif /* SPICE_CORE_H_ */
#endif /* SPICE_EXPERIMENTAL_H_ */
#endif /* SPICE_H_ */
#endif /* SPICE_INPUT_H_ */
#endif /* SPICE_MIGRATION_H_ */
#endif /* SPICE_QXL_H_ */
#endif /* SPICE_REPLAY_H_ */

/*** END file-tail ***/
#endif /* SPICE_SERVER_H_ */
#endif /* SPICE_VERSION_H_ */
#endif /* STAT_FILE_H_ */
#endif /* STAT_H_ */
#endif /* STREAM_CHANNEL_H_ */
#endif /* TREE_H_ */
#endif /* UTILS_H_ */
#endif /* VIDEO_ENCODER_H_ */
#endif /* VIDEO_STREAM_H_ */
#endif /* ZLIB_ENCODER_H_ */


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

2018-02-15 Thread Lukáš Hrázký 
On Wed, 2018-02-14 at 22:43 +0100, Christophe de Dinechin wrote:
> > On 14 Feb 2018, at 17:29, Christophe Fergeau  wrote:
> > 
> > This changes the suggested style to what is currently used in
> > spice-server codebase. This also removes a few sentences which
> > are not really related to how one should format their header guards.
> > 
> > Signed-off-by: Christophe Fergeau 
> > ---
> > docs/spice_style.txt | 6 ++
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> > index c8a4cff66..bc18b1d9e 100644
> > --- a/docs/spice_style.txt
> > +++ b/docs/spice_style.txt
> > @@ -365,12 +365,10 @@ Headers should be protected against multiple 
> > inclusion using a macro that contai
> > 
> > ...
> > 
> > -#endif // MY_MODULE_H
> > +#endif /* MY_MODULE_H */
> 
> I had first written it with C style, Frediano suggested C++ style.Both are 
> OK. Currently, we have 44 of one and 208 of the other, so the existing code 
> base does not imply one or the other.

FWIW, I see no reason to use /* */ when // is simpler.

Lukas

> > 
> > 
> > -The macro may include additional information, e.g. a component. For 
> > example a file generally referenced as foo/bar.h could use a FOO_BAR_H 
> > macro.
> 
> Nack the removal. We want a guideline, because right now it’s “anything goes” 
> and it looks ugly:
> 
> channel-smartcard.h:18:#ifndef __SPICE_CLIENT_SMARTCARD_CHANNEL_H__ (not a 
> legal name)
> channel-display-priv.h:18:#ifndef CHANNEL_DISPLAY_PRIV_H_
> spice-widget-priv.h:18:#ifndef __SPICE_WIDGET_PRIV_H__
> usb-acl-helper.h:21:#ifndef __SPICE_USB_ACL_HELPER_H__
> channel-usbredir-priv.h:21:#ifndef __SPICE_CLIENT_USBREDIR_CHANNEL_PRIV_H__
> generated_client_marshallers.h:23:#ifndef _H_GENERATED_CLIENT_MARSHALLERS
> spice-char.h:18:#ifndef SPICE_CHAR_H_
> net-utils.h:18:#ifndef RED_NET_UTILS_H_
> hexdump.h:6:#ifndef RH_STREAMING_AGENT_HEXDUMP_H_
> 
> 
> > -
> > -Historically, some headers added underscores liberally, e.g. MY_MODULE_H_. 
> > This is neither necessary nor discouraged, although as a reminder, a 
> > leading underscore followed by a capital letter is reserved for the 
> > implementation and should not be used, so _MY_MODULE_H is, technically 
> > speaking, invalid C.
> 
> Nack the removal, see example of channel-smartcard.h, which is not legal, 
> it’s reserved to the implementation, i.e. compiler and standard libraries.
> 
> 17.4.3.1.2 Global names [lib.global.names]
> 
> Certain sets of names and function signatures are always reserved to the 
> implementation:
> 
>   • Each name that contains a double underscore (__) or begins with an 
> underscore followed by an uppercase letter (2.11) is reserved to the 
> implementation for any use.
>   • Each name that begins with an underscore is reserved to the 
> implementation for use as a name in the global namespace.165
> 
> 
> > +C++ headers would use C++ comments.
> 
> There are no “C++ comments” anymore since C99.
> 
> > 
> > Header inclusion
> > 
> > -- 
> > 2.14.3
> > 
> > ___
> > 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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

2018-02-15 Thread Christophe Fergeau 
On Wed, Feb 14, 2018 at 10:43:26PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 14 Feb 2018, at 17:29, Christophe Fergeau  wrote:
> > 
> > This changes the suggested style to what is currently used in
> > spice-server codebase. This also removes a few sentences which
> > are not really related to how one should format their header guards.
> > 
> > Signed-off-by: Christophe Fergeau 
> > ---
> > docs/spice_style.txt | 6 ++
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> > index c8a4cff66..bc18b1d9e 100644
> > --- a/docs/spice_style.txt
> > +++ b/docs/spice_style.txt
> > @@ -365,12 +365,10 @@ Headers should be protected against multiple 
> > inclusion using a macro that contai
> > 
> > ...
> > 
> > -#endif // MY_MODULE_H
> > +#endif /* MY_MODULE_H */
> 
> I had first written it with C style, Frediano suggested C++ style.Both are 
> OK. Currently, we have 44 of one and 208 of the other, so the existing code 
> base does not imply one or the other.

spice/server/*.h is consistently using /* */, and this file is in
spice-server codebase. So I'd rather this reflects what is being done
there ;)

> 
> > 
> > 
> > -The macro may include additional information, e.g. a component. For 
> > example a file generally referenced as foo/bar.h could use a FOO_BAR_H 
> > macro.
> 
> Nack the removal. We want a guideline, because right now it’s “anything goes” 
> and it looks ugly:

There is a guideline at the top of the section before the example:
"Headers should be protected against multiple inclusion using a macro
that contains the header file name in uppercase, with all characters
that are invalid in C replaced with an underscore '_':"

So I'm not advocating for "anything goes", I'm removing some parts which
I don't think are bringing much, or could even be confusing.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

2018-02-14 Thread Christophe de Dinechin 


> On 14 Feb 2018, at 17:29, Christophe Fergeau  wrote:
> 
> This changes the suggested style to what is currently used in
> spice-server codebase. This also removes a few sentences which
> are not really related to how one should format their header guards.
> 
> Signed-off-by: Christophe Fergeau 
> ---
> docs/spice_style.txt | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index c8a4cff66..bc18b1d9e 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -365,12 +365,10 @@ Headers should be protected against multiple inclusion 
> using a macro that contai
> 
> ...
> 
> -#endif // MY_MODULE_H
> +#endif /* MY_MODULE_H */

I had first written it with C style, Frediano suggested C++ style.Both are OK. 
Currently, we have 44 of one and 208 of the other, so the existing code base 
does not imply one or the other.

> 
> 
> -The macro may include additional information, e.g. a component. For example 
> a file generally referenced as foo/bar.h could use a FOO_BAR_H macro.

Nack the removal. We want a guideline, because right now it’s “anything goes” 
and it looks ugly:

channel-smartcard.h:18:#ifndef __SPICE_CLIENT_SMARTCARD_CHANNEL_H__ (not a 
legal name)
channel-display-priv.h:18:#ifndef CHANNEL_DISPLAY_PRIV_H_
spice-widget-priv.h:18:#ifndef __SPICE_WIDGET_PRIV_H__
usb-acl-helper.h:21:#ifndef __SPICE_USB_ACL_HELPER_H__
channel-usbredir-priv.h:21:#ifndef __SPICE_CLIENT_USBREDIR_CHANNEL_PRIV_H__
generated_client_marshallers.h:23:#ifndef _H_GENERATED_CLIENT_MARSHALLERS
spice-char.h:18:#ifndef SPICE_CHAR_H_
net-utils.h:18:#ifndef RED_NET_UTILS_H_
hexdump.h:6:#ifndef RH_STREAMING_AGENT_HEXDUMP_H_


> -
> -Historically, some headers added underscores liberally, e.g. MY_MODULE_H_. 
> This is neither necessary nor discouraged, although as a reminder, a leading 
> underscore followed by a capital letter is reserved for the implementation 
> and should not be used, so _MY_MODULE_H is, technically speaking, invalid C.

Nack the removal, see example of channel-smartcard.h, which is not legal, it’s 
reserved to the implementation, i.e. compiler and standard libraries.

17.4.3.1.2 Global names [lib.global.names]

Certain sets of names and function signatures are always reserved to the 
implementation:

• Each name that contains a double underscore (__) or begins with an 
underscore followed by an uppercase letter (2.11) is reserved to the 
implementation for any use.
• Each name that begins with an underscore is reserved to the 
implementation for use as a name in the global namespace.165


> +C++ headers would use C++ comments.

There are no “C++ comments” anymore since C99.

> 
> Header inclusion
> 
> -- 
> 2.14.3
> 
> ___
> 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