[Spice-devel] [PATCH spice-server] style: Update style to include some C++ element

[Christophe de Dinechin]

> On 8 Feb 2018, at 14:53, Lukáš Hrázký <lhrazky at redhat.com> 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 at redhat.com>
>>> ---
>>> 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel