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

Thu Feb 15 11:59:01 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.
> 

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