Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()

From: Ian Campbell <ian.campbell@citrix.com>
To: Ian Jackson <Ian.Jackson@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>, Xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH] tools/toollog: Drop XTL_NEW_LOGGER()
Date: Wed, 20 Jan 2016 17:21:26 +0000	[thread overview]
Message-ID: <1453310486.26343.166.camel@citrix.com> (raw)
In-Reply-To: <22174.31033.369877.629267@mariner.uk.xensource.com>

On Tue, 2016-01-19 at 17:58 +0000, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH] tools/toollog: Drop
> XTL_NEW_LOGGER()"):
> > On 19/01/16 17:36, Ian Jackson wrote:
> > > I think this macro is useful because if you wanted to write (say)
> > > xtl_logger_syslog, you would want to use it to help you with some
> > > boilerplate.
> > 
> > WTF? Even documented, the behaviour of this macro is insane, which is
> > why I am trying to kill it.  After this, I will also be fixing the
> > gross
> > pointer abuse which exists in the xentoollog internals, before the ABI
> > becomes fixed in 4.7.
> 
> I think the behaviour of this macro is perfectly sane.

It's clearly not "insane", that's for sure, and statements of such opinions
along those lines in the commit message (as oppose to concrete issues) are
why I found the original patch unsuitable to apply.

> I think your reference to `gross pointer abuse' is to the casting from
> the specific to the generic struct.  This is a completely standard
> technique for oopy stuff in C.  Here is a whole library (quite a nice
> neat library, in fact) that uses it:
>    http://www.lysator.liu.se/liboop/
>    http://www.lysator.liu.se/liboop/ref.html
> 
> > Irrespective of whether you disagree with my opinions here,
> > xentoollog.h
> > is specified to be C99 -strict, meaning no GNUisms.
> 
> Specified where ?

Sort of in "tools: Arrange to check public headers for ANSI compatiblity"
(yet to be applied) where I add checks that the headers are clean per gcc's
-ansi flag, which is equivalent to -std=c90.

This was done to support users of the library on platforms with lagging
compilers, specifically MS VCC lacks some stuff more modern C stuff (like
variable length arrays IIRC, the details were in one of the threads on that
patch)

This check doesn't cover #defines though.

> Anyway, there is no requirement to use this macro.  If someone wants
> to write a strict C99 xtl logger then they can do it by hand.  (I
> predict that no-one will want to do that.)  So there is clearly no
> actual reason why this macro ought to be pure C99.

Now that the macro is documented I think the GNUism is the only remaining
concern which has been put forward which I think needs addressing any
further and I agree with the logic that it is a #define so if you want to
use an non-gnu compiler you don't get the handy macro.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel