> On 8 Feb 2018, at 15:48, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote: > > 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? I’ve not tried on whole files. In my experience, it does not work. But as integrated in Emacs, for reformatting code, it generally does a rather good job. > > 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. Well, it’s empty code with nothing in it, so both before and after look weird. Now, when I look at it from a distance, the original looks very top heavy with some funny stuff dangling at the bottom. The reformatted one looks much more balanced and well-spaced. So I guess I disagree with what you probably thought was an obvious conclusion but is really a personal preference. YMMV. BTW, while I’m at it, about spacing and comments: https://grenouillebouillie.wordpress.com/2017/12/09/the-alsys-ada-commenting-style/. Cheers, Christophe > >>> >>>>> + >>>>> +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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel