Re: [PATCH 1/3] improve detection of attribute conflicts (PR 81544)

[Joseph Myers <joseph at codesourcery dot com>]

On Thu, 17 Aug 2017, Martin Sebor wrote:

> +/* Check LAST_DECL and NODE of the same symbol for attributes that are
> +   recorded in EXCL to be mutually exclusive with ATTRNAME, diagnose
> +   them, and return true if any have been found.  NODE can be a DECL
> +   or a TYPE.  */
> +
> +static bool
> +diag_attr_exclusions (tree last_decl, tree node, tree attrname,
> +		      const attribute_spec *spec)

EXCL is not an argument to this function, so the comment above it should 
not refer to EXCL (presumably it should refer to SPEC instead).

> +	    note &= warning (OPT_Wattributes,
> +			     "ignoring attribute %qE in declaration of "
> +			     "a built-in function qD because it conflicts "
> +			     "with attribute %qs",
> +			     attrname, node, excl->name);

%qD not qD, presumably.

(Generically, warning_at would be preferred to warning, but that may best 
be kept separate if you don't already have a location available here.)

> +static const struct attribute_spec::exclusions attr_gnu_inline_exclusions[] =
> +{
> +  ATTR_EXCL ("gnu_inline", true, true, true),
> +  ATTR_EXCL ("noinline", true, true, true),
> +  ATTR_EXCL (NULL, false, false, false),
> +};

This says gnu_inline is incompatible with noinline, and is listed as the 
EXCL field for the gnu_inline attribute.

> +static const struct attribute_spec::exclusions attr_inline_exclusions[] =
> +{
> +  ATTR_EXCL ("always_inline", true, true, true),
> +  ATTR_EXCL ("noinline", true, true, true),
> +  ATTR_EXCL (NULL, false, false, false),
> +};

This is listed as the EXCL field for the noinline attribute, but does not 
mention gnu_inline.  Does this mean some asymmetry in when that pair is 
diagnosed?  I don't see tests for that pair added by the patch.

(Of course, gnu_inline + always_inline is OK, and attr_inline_exclusions 
is also used for the always_inline attribute in this patch.)

In general, the data structures where you need to ensure manually that if 
attribute A is listed in EXCL for B, then attribute B is also listed in 
EXCL for A, seem concerning.  I'd expect either data structures that make 
such asymmetry impossible, or a self-test that verifies that the tables in 
use are in fact symmetric (unless there is some reason the symmetry is not 
in fact required and symmetric diagnostics still result from asymmetric 
tables - in which case the various combinations and orderings of 
gnu_inline and noinline definitely need tests to show that the diagnostics 
work).

> +both the @code{const} and the @code{pure} attribute is diagnnosed.

s/diagnnosed/diagnosed/

-- 
Joseph S. Myers
joseph@codesourcery.com