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

[Christophe de Dinechin]

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

?

> 
>> 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 :-)

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

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

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



> 
> Frediano