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@xxxxxxxxxx>
> >
> > * 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
Attachment:
pgp6OGNlqITE_.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list