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

Submitted by Christophe Fergeau on Feb. 14, 2018, 4:29 p.m.

Details

Message ID 20180214162931.31861-1-cfergeau@redhat.com
State New
Headers show
Series "style: Slight tweak to the header guard section" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe Fergeau Feb. 14, 2018, 4:29 p.m.
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 <cfergeau@redhat.com>
---
 docs/spice_style.txt | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

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 */
 ----
 
-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.
-
-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.
+C++ headers would use C++ comments.
 
 Header inclusion
 ----------------

Comments

> On 14 Feb 2018, at 17:29, Christophe Fergeau <cfergeau@redhat.com> 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 <cfergeau@redhat.com>
> ---
> 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
On Wed, Feb 14, 2018 at 10:43:26PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 14 Feb 2018, at 17:29, Christophe Fergeau <cfergeau@redhat.com> 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 <cfergeau@redhat.com>
> > ---
> > 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
On Wed, 2018-02-14 at 22:43 +0100, Christophe de Dinechin wrote:
> > On 14 Feb 2018, at 17:29, Christophe Fergeau <cfergeau@redhat.com> 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 <cfergeau@redhat.com>
> > ---
> > 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
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 <cfergeau@redhat.com> 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 <cfergeau@redhat.com>
> > > ---
> > > 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_ */
> 
> 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 <cfergeau@redhat.com>
> > > > 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 <cfergeau@redhat.com>
> > > > ---
> > > > 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
> On 15 Feb 2018, at 11:43, Frediano Ziglio <fziglio@redhat.com> 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 <cfergeau@redhat.com>
>>>>> 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 <cfergeau@redhat.com>
>>>>> ---
>>>>> 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 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.

Also, since Christophe was not there at the beginning of the discussion, I think that the consensus is that guidelines are what we want, not what is there. If what is there is inconsistent, we’ll slowly fix it over time.

> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> > On 15 Feb 2018, at 11:43, Frediano Ziglio <fziglio@redhat.com> 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 <cfergeau@redhat.com>
> >>>>> 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 <cfergeau@redhat.com>
> >>>>> ---
> >>>>> 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 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.
> 
> Also, since Christophe was not there at the beginning of the discussion, I
> think that the consensus is that guidelines are what we want, not what is
> there. If what is there is inconsistent, we’ll slowly fix it over time.
> 

At the beginning there was the void. Then antimatter separate from matter
and the big bang happened... maybe too early ? :-D

Well, the beginning was a mail from Lukash attempting to define some
specific aspect of C++ like method naming and namespace.
We were talking on how to integrate these changes and was agreed to
put into spice-server doc so we didn't have to change generation
and publication of this document.
From that beginning I think the situation went a bit out of control.

Frediano
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
> On 15 Feb 2018, at 13:41, Christophe Fergeau <cfergeau@redhat.com> 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
On Thu, Feb 15, 2018 at 03:25:23PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 15 Feb 2018, at 13:41, Christophe Fergeau <cfergeau@redhat.com> 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
> On 15 Feb 2018, at 15:55, Christophe Fergeau <cfergeau@redhat.com> wrote:
> 
> On Thu, Feb 15, 2018 at 03:25:23PM +0100, Christophe de Dinechin wrote:
>> 
>> 
>>> On 15 Feb 2018, at 13:41, Christophe Fergeau <cfergeau@redhat.com> 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
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