[Spice-devel] [PATCH spice-server] style: Update style to include some C++ element
Christophe de Dinechin
dinechin at redhat.com
Fri Feb 9 10:36:35 UTC 2018
> On 8 Feb 2018, at 15:48, Lukáš Hrázký <lhrazky at redhat.com> 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 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.
>
> 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 at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list