[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 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.
For instance we have a section that explicitly state we don't
want column alignment.
Or see the section about section declaration.

> 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.
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.

> - 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.

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.

Frediano