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

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

 



> 
> 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@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.
> 

As you said OT, good for a follow up.
No, is not completely backwards, just that the "main" header should be after
the configure. I would suggest this also for C.

> >  ----
> > +
> > +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:
> 

About the later variants why not saying C++11 is not enough?
Or something like "For compatibility reason don't use more recent C++ dialects." ?

> - 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)
> 

Our C code use CamelCase for structures.

> > +
> > +
> > +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.
> 

Yes, I'll update it.

> > +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.
> 
> > +
> > +class ClassInsideNamespace
> > +{

I think to be consistent with C style the:

class ClassInsideNamespace {

should be used. Not clear how this play with inheritance :-(

class ClassInsideNamespace : public Whatever {

(the problem happens when the line is quite long)

> > +...
> > +};
> > +
> > +}} // 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;
> 
> 

I'll update also this

Frediano
_______________________________________________
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]