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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> 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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]