> > > On 15 Feb 2018, at 11:59, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote: > > > >>> > >>> On 15 Feb 2018, at 10:07, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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@xxxxxxxxxx> > >>>>> 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel