[Spice-devel] [PATCH spice-server] style: Update style to include some C++ element
Lukáš Hrázký
lhrazky at redhat.com
Thu Feb 8 14:48:49 UTC 2018
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?
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.
> >
> > > > +
> > > > +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
>
>
More information about the Spice-devel
mailing list