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

Thu Feb 15 10:55:44 UTC 2018

> On 15 Feb 2018, at 11:43, Frediano Ziglio <fziglio at 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 at 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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel