[Spice-devel] [PATCH v3 01/11] Add .clang-format with defaults matching what's specified in the style guide

[Frediano Ziglio]

> 
> > On 15 Feb 2018, at 11:59, Frediano Ziglio <fziglio at redhat.com> wrote:
> > 
> >>> 
> >>> On 15 Feb 2018, at 10:07, Christophe Fergeau <cfergeau at redhat.com> wrote:
> >>> 
> >>> On Wed, Feb 14, 2018 at 10:29:25PM +0100, Christophe de Dinechin wrote:
> >>>> 
> >>>> 
> >>>>> On 14 Feb 2018, at 17:34, Christophe Fergeau <cfergeau at redhat.com>
> >>>>> wrote:
> >>>>> 
> >>>>> On Wed, Feb 14, 2018 at 10:45:56AM -0500, Frediano Ziglio wrote:
> >>>>>>> 
> >>>>>>> Shouldn't this go with a Makefile rule? A few lines in the log what
> >>>>>>> this
> >>>>>>> is about, what is the goal for having this file, ... would not hurt.
> >>>>>>> 
> >>>>>>> Christophe
> >>>>>>> 
> >>>>>> 
> >>>>>> I think this file is supposed to just help developers so should not
> >>>>>> be in the Makefile.
> >>>>> 
> >>>>> Yes, after reading various threads, it's apparently meant to be used
> >>>>> together with emacs for formatting of small code blocks, it's not
> >>>>> usable
> >>>>> on the whole codebase. So a 'make clang-format' rule apparently would
> >>>>> not make sense.
> >>>>> 
> >>>>>> I think you mean that the intention should be written in the commit
> >>>>>> message.
> >>>>> 
> >>>>> Yes, knowing how it's meant to be used, why we want it in the codebase.
> >>>> 
> >>>> Why would we NOT want it in the codebase?
> >>> 
> >>> I'm not saying I'm against what this patch is adding. I'm saying
> >>> I'm against a commit adding something without much of a rationale in its
> >>> commit log.
> >> 
> >> Fair point. Added that.
> >> 
> >> To clarify, right now, I don’t think we can refactor whole files yet, the
> >> code is not ready for it. See
> >> https://gitlab.com/c3d/spice-server/commit/f6297e63f5e785e5fdf7176fd9381d0f465f6c11
> >> for a review of the server changes, if you are curious.
> >> 
> > 
> > I think the main reason is that you are attempting to change the
> > current style entirely.
> 
> No, really not.
> 
> > For instance we have a section that explicitly state we don't
> > want column alignment.
> 
> Yes, ooops. That was not in the branch when I originally pushed it for
> review, and then I experimented with that setting and did a push -f.
> 
> > Or see the section about section declaration.
> 
> ?
> 

Sorry, the function declaration/definitions.

> > 
> >> Some of the issues include:
> >> 
> >> - Our headers are not yet self-contained, so you get:
> >> 	In file included from agent-msg-filter.c:25:
> >> 	In file included from ./red-common.h:37:
> >> 	In file included from ./spice.h:27:
> >> 	./spice-migration.h:47:31: error: unknown type name ‘SpiceServer'
> >> 
> > 
> > I test quite often and as far as I know they are self contained.
> 
> I just proved they are not :-)
> 

They are designed like glib headers, you have to include spice.h,
the only exception is the server code which has however to include
them in a proper order.

> > In this case spice-migration.h should be included from spice.h
> > and SpiceServer is defined in spice-server.h included from spice.h
> > before spice-migration.h.
> 
> That is the very definition of “not self contained"
> 

Yes, is the exception that prove the rule.

> > 
> >> - Some comments get munged, notably comments at end of line:
> >> 
> >> - Minor things, like a macro where (id) & CONSTANT interpreted as a cast
> >> (id)
> >> with address taken and reformatted accordingly
> >> 
> >> Christophe
> > 
> > If your intention with this file is changing the entire style
> > I would personally avoid it.
> 
> No. I did this experiment to get a feel of how close I was to the original
> style (assuming there really is one, which is not obvious from reading the
> code).
> 

Ok, then you post the wrong link to prove your point.

> > 
> > The subject is "Add .clang-format with defaults matching what's specified
> > in the style guide", from the branch you pointed out looks like is false.
> 
> Not, it looks like it is not yet true ;-)
> 
> For instance, I noticed by doing this experiment that “Linux” is wrong, we
> want “Mozilla”. Also, I think we would need BinPackParameters: false. I
> experimented with the alignment to try to fix issues I was observing with
> the alignment of parameters, but as you point out, it also aligns decls. I
> personally like that, but that’s not what the style guide says.
> 

There are a bit of inconsistency, indeed.
In some file some more "gobject/glib" style was followed (like column
indentation or parameter indentation).
That's why defining a "we'd like this style" but not be too restrictive
and more respectful of current code avoids to change everything.

Frediano