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

[Lukáš Hrázký]

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.
> > See discussion "Coding style and naming conventions for C++" at
> > https://lists.freedesktop.org/archives/spice-devel/2018-January/041562.html.
> > 
> > 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.
> 
> > +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.

Or, well, not entirely agreeing on this one. For consistency sake, yes.

Otherwise, it only adds visual clutter for me, adding in this case 2
useless lines, which you have to scroll to get to the meat. But then
again, I find it the same with classes and functions... :)

> > +
> > +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;
> 
> 
> --
> Cheers,
> Christophe de Dinechin (IRC c3d)
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel