[libvirt] [PATCH libvirt-glib 1/5] Document some of the coding style conventions required

[Christophe Fergeau]

On Mon, Nov 28, 2011 at 05:24:56PM +0100, Christophe Fergeau wrote:
> On Mon, Nov 28, 2011 at 01:13:44PM +0000, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange at redhat.com>
> > 
> > * HACKING: Add notes on coding style conventions
> > ---
> >  HACKING |  221 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 220 insertions(+), 1 deletions(-)
> > 
> > diff --git a/HACKING b/HACKING
> > index fe52c17..ea419e5 100644
> > --- a/HACKING
> > +++ b/HACKING
> > @@ -6,4 +6,223 @@ having to run make install, there are two environment variables
> >  that need to be set:
> >  
> >     export GI_TYPELIB_PATH=`pwd`/libvirt-glib:`pwd`/libvirt-gconfig:`pwd`/libvirt-gobject
> > -   export LD_LIBRARY_PATH=`pwd`/libvirt-glib/.libs:`pwd`/libvirt-gconfig/.libs:`pwd`/libvirt-gobject/.libs
> > \ No newline at end of file
> > +   export LD_LIBRARY_PATH=`pwd`/libvirt-glib/.libs:`pwd`/libvirt-gconfig/.libs:`pwd`/libvirt-gobject/.libs
> > +
> > +
> > +Coding style rules
> > +
> > + - Opening & closing braces for functions should be at start of line
> > +
> > +     int foo(int bar)
> > +     {
> > +       ...
> > +     }
> > +
> > +   Not
> > +
> > +     int foo(int bar) {
> > +       ...
> > +     }
> > +
> > +
> > +
> > + - Opening brace for if/while/for loops should be at the end of line
> > +
> > +     if (foo) {
> > +        bar;
> > +        wizz;
> > +     }
> > +
> > +   Not
> > +
> > +     if (foo)
> > +     {
> > +        bar;
> > +        wizz;
> > +     }
> > +
> > +   Rationale: putting every if/while/for opening brace on a new line
> > +   expands function length too much
> > +
> > +
> > +
> > + - If a brace needs to be used for one clause in an if/else statement,
> > +   it should be used for both clauses, even if the other clauses are
> > +   only single statements. eg
> > +
> > +      if (foo) {
> > +         bar;
> > +         wizz;
> > +      } else {
> > +         eek;
> > +      }
> > +
> > +   Not
> > +
> > +      if (foo) {
> > +         bar;
> > +         wizz;
> > +      } else
> > +         eek;
> > +
> > +
> > +
> > + - Function parameter attribute annotations should follow the parameter
> > +   name, eg
> > +
> > +      int foo(int bar G_GNUC_UNUSED)
> > +      {
> > +      }
> > +
> > +   Not
> > +
> > +      int foo(G_GNUC_UNUSED int bar)
> > +      {
> > +      }
> > +
> > +   Rationale: Adding / removing G_GNUC_UNUSED  should not cause the
> > +   rest of the line to move around since that obscures diffs.
> > +
> > +
> > +
> > + - There should be no space between function names & open brackets eg
> > +
> > +      int foo(int bar)
> > +      {
> > +      }
> > +
> > +   Not
> > +
> > +      int foo (int bar)
> > +      {
> > +      }
> > +
> > +
> > +
> > + - To keep lines under 80 characters (where practical), multiple parameters
> > +   should be on new lines. Do not attempt to line up parameters vertically
> > +   eg
> > +
> > +      int foo(int bar,
> > +              unsigned long wizz)
> > +      {
> > +      }
> > +
> > +   Not
> > +
> > +      int foo(int bar, unsigned long wizz)
> > +      {
> > +      }
> > +
> > +   Not
> > +
> > +      int foo(int           bar,
> > +              unsigned long wizz)
> > +      {
> > +      }
> > +
> > +    Rationale: attempting vertical alignment causes bigger diffs when
> > +    modifying code if type names change causing whitespace re-alignment.
> > +
> > +
> > +
> > + - Function declarations in headers should not attempt to line up parameters
> > +   vertically
> > +
> > +    eg
> > +
> > +        int foo(int bar)
> > +        unsigned long eek(sometype wizz)
> > +
> > +    Not
> > +
> > +
> > +        int           foo(int      bar)
> > +        unsigned long eek(sometype wizz)
> > +
> > +   Rationale: when making changes to headers, existing vertical alignment
> > +   is often invalidated, requiring reindentation. This causes bigger
> > +   diffs which obscures code review.
> > +
> > +   Exception: the 6 common decls for defining a new GObject
> > +
> > +    #define GVIR_TYPE_INPUT_STREAM            (_gvir_input_stream_get_type ())
> > +    #define GVIR_INPUT_STREAM(inst)           (G_TYPE_CHECK_INSTANCE_CAST ((inst), GVIR_TYPE_INPUT_STREAM, GVirInputStream))
> > +    #define GVIR_INPUT_STREAM_CLASS(class)    (G_TYPE_CHECK_CLASS_CAST ((class), GVIR_TYPE_INPUT_STREAM, GVirInputStreamClass))
> > +    #define GVIR_IS_INPUT_STREAM(inst)        (G_TYPE_CHECK_INSTANCE_TYPE ((inst), GVIR_TYPE_INPUT_STREAM))
> > +    #define GVIR_IS_INPUT_STREAM_CLASS(class) (G_TYPE_CHECK_CLASS_TYPE ((class), GVIR_TYPE_INPUT_STREAM))
> > +    #define GVIR_INPUT_STREAM_GET_CLASS(inst) (G_TYPE_INSTANCE_GET_CLASS ((inst), GVIR_TYPE_INPUT_STREAM, GVirInputStreamClass))
> > +
> > +   Rationale: These decls never change once added so vertical
> > +   alignment of these 6 lines only, will not cause diff whitespace
> > +   problems later.
> > +
> > +
> > + - Usage of goto should follow one of the following patterns, and
> > +   label naming conventions. In particular any exit path jumps should
> > +   obay the 'cleanup' vs 'error' label naming
> > +
> > +   * Interrupted system calls:
> > +
> > +      retry:
> > +        err = func()
> > +        if (err < 0 && errno == EINTR)
> > +            goto retry;
> > +
> > +     Alternate label name:  retry_func:
> > +
> > +
> > +   * Shared cleanup paths:
> > +
> > +       int foo(int bar)
> > +       {
> > +          int ret = -1;
> > +
> > +
> > +          if (something goes wrong)
> > +             goto cleanup;
> > +
> > +          ret = 0;
> > +        cleanup:
> > +          ...shared cleanup code...
> > +          return ret;
> > +       }
> > +
> > +
> > +   * Separate error exit paths:
> > +
> > +       int foo(int bar)
> > +       {
> > +          if (something goes wrong)
> > +             goto error;
> > +
> > +          return 0;
> > +
> > +        error:
> > +          ...error cleanup code...
> > +          return -1;
> > +       }
> > +
> > +
> > +   * Separate and shared error exit paths:
> > +
> > +       int foo(int bar)
> > +       {
> > +          int ret = -1;
> > +
> > +          if (something very bad goes wrong)
> > +             goto error;
> > +
> > +          if (something goes wrong)
> > +             goto cleanup;
> > +
> > +          ret = 0;
> > +        cleanup:
> > +          ...shared cleanup code...
> > +          return 0;
> > +
> > +        error:
> > +          ...error cleanup code...
> > +          return -1;
> > +       }
> > +
> 
> For what it's worth this adds a blank line to the end of the file.

And ACK in any case

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111128/999c22fd/attachment-0001.sig>