Re: [Xen-devel] [PATCH 12/15] libxl: Use GC_INIT and GC_FREE everywhere

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 12/15] libxl: Use GC_INIT and 
GC_FREE everywhere"):
> On Mon, 2011-12-05 at 18:10 +0000, Ian Jackson wrote:
> >     GC_FREE;
> 
> I suppose this really relates to the earlier patch which adds these
> macros but wouldn't "GC_FREE();" be nicer?

I don't normally write zero-adic statement macros that way.  I guess
this is just one of those stylistic points like whether to put parens
around the argument to return.

We don't currently have any other zero-adic statement macros in libxl,
nor any macros which depend on or introduce names in the caller's
namespace, apart from those we inherit from flex.

My personal view is that non-hygienic statement macros are unusual
enough that it's worth drawing attention to them by using a special
syntax.  After all, their semantics are not normally very like
functions because they refer to objects, including local variables,
not explicitly named in the call.  Often they declare variables or
labels, and those kinds of statements don't contain any ().  So I
think the empty () are misleading - they hint that the thing is a
function, when it really isn't enough like a function.

In the rest of the xen tree we seem to have these without the "()" [1]:
    tools/libxen/test/test_event_handling.c:        CLEANUP;
    tools/blktap2/drivers/lock.c:                XSLEEP;
and these with the "()" [2]:
    xen/drivers/acpi/osl.c:         BUG();
    xen/arch/ia64/asm-offsets.c:    BLANK();
    xen/common/inflate.c:    NEXTBYTE();

Of these:

 CLEANUP    non-hygienic block
 NEXTBYTE() non-hygienic block wrapped up in () to make it an expression
 XSLEEP     hygienic expression (a call to usleep)
 BUG()      hygienic expression (a call to asm)
 BLANK()    something in some kind of table, not really relevant here

This doesn't seem to provide any consistent rule.

Perhaps the right answer is a patch to the coding standard.  One
option for this below.  This is a bit of a bikeshed issue of course.

Ian.

[1] find * -name \*.c | xargs egrep '^[   ]*[A-Z][A-Z]*\;' |less
[2] find * -name \*.c | xargs egrep '^[   ]*[A-Z][A-Z]*\(\)' |less
  in both cases only one hit per macro mentioned


diff --git a/tools/libxl/CODING_STYLE b/tools/libxl/CODING_STYLE
index 110a48f..69fedb9 100644
--- a/tools/libxl/CODING_STYLE
+++ b/tools/libxl/CODING_STYLE
@@ -133,3 +133,25 @@ Rationale: a consistent (except for functions...) bracing 
style reduces
 ambiguity and avoids needless churn when lines are added or removed.
 Furthermore, it is the libxenlight coding style.
 
+
+6. Macros
+
+Naturally, macros (other than function-like ones which can be used
+just like functions eg evaluate their arguments each exactly once
+etc.) should be named in ALL_CAPITALS.  Expansions, and references to
+arguments should be protected with appropriate ( ), and code
+with appropriate do{ ... }while(0) blocks.
+
+A zero-argument macro which expands to a non-constant expression, or
+to an ordinary block, should be defined with empty parens like this:
+  #define MACRO() ....
+
+A zero-argument macro which expands to a constant expression; or whose
+expansion relies on (or introduces) declarations, labels, etc., in the
+containing scope; or which expands to another kind of code fragment,
+should be defined like this:
+  #define MACRO ...
+
+Macros expanding to declarations and/or statements should not include
+any trailing ";" so that they may be used like this:
+  MACRO(arguments);

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel