[apparmor] [patch] C tools: rename __unused macro

[Steve Beattie]

On Tue, Sep 23, 2014 at 12:35:01PM -0700, Seth Arnold wrote:
> On Mon, Sep 22, 2014 at 06:55:26PM -0700, Steve Beattie wrote:
> > On Thu, Sep 11, 2014 at 07:49:36PM +0200, Christian Boltz wrote:
> > > Am Donnerstag, 11. September 2014 schrieb Steve Beattie:
> > > > Bug: https://bugzilla.novell.com/show_bug.cgi?id=895495
> > > > 
> > > > We define the __unused macro as a shortcut for __attribute__((unused))
> > > > to quiet compiler warnings for functions where an argument is unused,
> > > > for whatever reason. However, on 64 bit architectures, older glibc's
> > > > bits/stat.h header defines an array variable with the name __unused
> > > > that collides with our macro and causes the parser to fail to build,
> > > > because the resulting macro expansion generates invalid C code.
> > > > 
> > > > This patch renames the macro to __aa_unused, 
> > > 
> > > >From https://bugzilla.novell.com/show_bug.cgi?id=895495#c4
> > > 
> > >     it's invalid to use the implementation namespace (two leading 
> > >     underscores).
> > > 
> > > That's exactly what we have - with or without your patch. The little 
> > > difference is that your patch adds an additional "aa" to make it less 
> > > likely to clash with existing names.
> > > 
> > > Wouldn't it be a better idea to use something that does _not_ start with 
> > > two underscores?
> > 
> > Yes and no. Yes because it could potentially clash with some
> > compiler/system symbol that begins with __aa_, which, while unlikely is
> > possible. No, because the use of underscores gives a hint to a code
> > reader that it has a special meaning, and isn't just a weird complex
> > type or variable.
> > 
> > Anyway, attached is an updated version of the patch that converts it to
> > aa_unused, as well as making the use of it consistent by having it be
> > the last part of the variable declaration; i.e. "type var aa_unused",
> > rather than "aa_unused type var" or "type aa_unused var".
> 
> I preferred the __aa_unused to aa_unused; if we're going to follow the
> letter of the spec and ditch the leading underscores I think I'd rather
> follow the lead of other projects and go capitalized, e.g. AA_UNUSED, but
> that's pretty deep in pink vs chartruse bikeshedding land, so meh.

Right, a common convention is to capitalize macros, to make them
distinct from regular symbols.

> > A second patch is attached that applies on top of that and adds a
> > second macro function called aa_unused_arg(), that wraps the variable
> > declaration name. so the above would be 'type aa_unused_arg(var)'.
> > I don't have a strong feeling either way about the second patch.
> 
> I don't like this one. Sorry. :)

Not at all. Honestly, my preferences are:

1) __unused — simple, clear, straightforward annotation. Alas,
   conflicts with a symbol in older glibcs and with convention of __
   prefixed symbols being system synbols. I wish gcc just went ahead
   and defined this.

2) __aa_unused — nearly as simple as __unused; the underscores
   indicate specialness, the __aa_ prefix makes it less likely to
   conflict with any actual system symbol name, despite intruding on
   the system symbol convention. NACK'ed by Christian, so would need
   at least one more ACK to be acceptable. (_aa_unused has the same
   problem, standards-wise,)

3) aa_unused — easy to confuse with the rest of an argument declaration.

4) aa_unused_arg() — mildly confusing in argument lists, but makes it
   more explicit that it's not to be confused with the type or name
   declaration.

5) AA_UNUSED — so shouty, hurts my ears and eyes. But follows the
   all-caps convention for macros, so is explicit about what it is.

Sooo, a great big shrug from me. But I'm getting tired of jenkins
nagging me that the parser fails to build on Ubuntu 12.04.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140924/f84cb310/attachment.pgp>