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