On Thu, 2018-02-08 at 15:33 +0100, Christophe de Dinechin wrote: > > On 8 Feb 2018, at 14:53, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote: > > > > On Wed, 2018-02-07 at 10:10 +0100, Christophe de Dinechin wrote: > > > Frediano Ziglio writes: > > > > > > > These style are used by other SPICE projects like spice-streaming-agent. > > > > "This style is used ..." > > > > > > See discussion "Coding style and naming conventions for C++" at > > > > https://lists.freedesktop.org/archives/spice-devel/2018-January/041562.html. > > > > I'm not sure if referring to ML discussion is a good idea, but if you > > think so, not objecting... :) > > > > > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx> > > > > --- > > > > docs/spice_style.txt | 38 +++++++++++++++++++++++++++++++++++++- > > > > 1 file changed, 37 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/docs/spice_style.txt b/docs/spice_style.txt > > > > index eb0e30ef..0e0028e9 100644 > > > > --- a/docs/spice_style.txt > > > > +++ b/docs/spice_style.txt > > > > @@ -1,7 +1,7 @@ > > > > Spice project coding style and coding conventions > > > > ================================================= > > > > > > > > -Copyright (C) 2009-2016 Red Hat, Inc. > > > > +Copyright (C) 2009-2018 Red Hat, Inc. > > > > Licensed under a Creative Commons Attribution-Share Alike 3.0 > > > > United States License (see http://creativecommons.org/licenses/by-sa/3.0/us/legalcode). > > > > > > > > @@ -379,3 +379,39 @@ Also in source (no header) files you must include `config.h` at the beginning so > > > > > > > > #include "spice_server.h" > > > > > > (Not in your patch) Re-reading the part about headers, it has everything > > > completely backwards! I suggest we re-discuss this. > > > > > > > ---- > > > > + > > > > +C++ > > > > +--- > > > > +C++ follows C style if not specified otherwise. > > > > > > I would add > > > > > > The C++11 dialect is assumed by default. No attempts will be made to > > > restrict the code to older variants of C++. Using constructs allowed in > > > later variants of C++ will be allowed either: > > > > > > - After the default C++ compiler for building the oldest supported RHEL > > > version fully supports the corresponding variant of C++ > > > > > > - Under the appropriate preprocessor conditions, if the previous > > > condition is not met and C++11 alternatives would have significant > > > drawbacks > > > > > > > > > > + > > > > +Method names > > > > +~~~~~~~~~~~~ > > > > + > > > > +Method names should use lower case and separate words with > > > > +underscores. > > > > > > What about classes? I see below that you selected camelcase for classes, > > > but that is inconsistent with standard C++ classes such as 'string', or > > > with most types in C (e.g. ptrdiff_t, struct stat, etc) > > > > > > Since it seems that consistency with C and with the standard C++ library > > > was the primary rationale for using snake-case, I would go for > > > snake-case for classes as well. > > > > > > (Frankly, I really could not care less, I'm just trying to be consistent) > > > > > > > + > > > > + > > > > +Namespaces > > > > +~~~~~~~~~~ > > > > + > > > > +Namespaces should use lower case and separate words with underscores. > > > > +Namespace blocks should not increase indentation. > > > > +Namespaces can be nested and closure can be collapsed but for > > > > > > I would not use the word "closure" here, which has a specific meaning in > > > computer science (as in lambda variable capture). What about: > > > > > > Namespaces can be nested. Namespace closing brackets for nested > > > namespaces can be placed together on the same line, but for readability > > > reasons the closure should specify the namespace with a comment. > > > > Agreed. > > > > > > +readability reasons the closure should specify the namespace with a > > > > +comment. > > > > + > > > > +[source,cpp] > > > > +---- > > > > +namespace spice { > > > > +namespace streaming_agent { > > > > > > There is a style inconsistency regarding the placement of the opening > > > brace relative to other first-column opening braces (function and > > > class). So I would suggest that all top-level opening braces be on a > > > line of their own, as for 'class' below and for functions. > > > > I'm not a fan, but if you guys decided you want to waste some lines on > > lonely opening braces :), please amend the patch. > > I’m more concerned about being able to clang-format a region than about discussing brace placement. > The “Linux” placement style on this: > > namespace a { > namespace b { > > class foo { > class bar { > foo() { > if (a) { b; c; } > if (b) { > c(); > d(); > e(); > } > } > }; > }; > > }} > > will give that: > > namespace a > { > namespace b > { > > class foo > { > class bar > { > foo() > { > if (a) { > b; > c; > } > if (b) { > c(); > d(); > e(); > } > } > }; > }; > } > } > > Don’t break automated tools gratuitously. If you know how to get clang-format to do what you want, I don’t care. That's the problem I have with code-formatting tools. I just can't get them to do what I want. I thought you could do that when you started talking about clang-format :D Sorry about that, anyway. I would think, though, that the bracket placement wouldn't be hard to configure. I have "have a look at clang-format" on my TODO list, item nr. 43 and increasing :( Do you actually successfully use clang-format on some SPICE code already? Lukas PS: Let's take a few seconds to consider how your example code looked before and after the format :D I just can't help it, I really dislike this format. > > > > > > + > > > > +class ClassInsideNamespace > > > > +{ > > > > +... > > > > +}; > > > > + > > > > +}} // namespace spice::streaming_agent > > > > +---- > > > > + > > > > +You should not import an entire namespace but use qualified names > > > > +instead. > > > > > > The C++ terminology is "using namespace" and not "import". So I would > > > rephrase as: > > > > > > The "using namespace" construct should never be used in headers. It should > > > be used sparingly in source files, and only within the body of short > > > functions. > > > > > > Preferred alternatives to "using namespace" include: > > > - using declarations > > > using spice::streaming_agent::some_class; > > > - namespace aliases > > > namespace ssa = spice::streaming_agent; > > > > Agreed as well. > > > > > -- > > > Cheers, > > > Christophe de Dinechin (IRC c3d) > > > _______________________________________________ > > > Spice-devel mailing list > > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel > > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel