Re: [PATCH] PR 53528 c++/ C++11 Generalized Attribute support

[Jason Merrill <jason at redhat dot com>]

On 08/15/2012 03:43 AM, Dodji Seketeli wrote:
> > Or we could just require people to put the attribute in the right
> > place (or one of the right places) if they want it to apply to the
> > decl.  That is, either at the beginning of the declaration statement
> > or after the declarator-id.
>
> Just to make sure I understand, you mean, for tagged types?  Because
> e.g for scalars, I feel like having is within the spirit of [dcl.spec]/1
>
>      const int [[some-attr]] foo = 10;
>
> (with [[some-attr]] applying to the integer type)
>
> right?

Yes, in this declaration [[some-attr]] applies to the type.  Whether or 
not that makes sense depends on the attribute and the type. 
Specifically, it doesn't ever make sense for classes, and I'm resistant 
to doing it for any types until we clarify how attributes participate in 
the type system.  C++11 doesn't clarify that because it doesn't have any 
attributes that can be used in this context.

I think we should start out with a fairly strict interpretation of the 
rules, and possibly relax them later, rather than try to relax them now.

> > > For unused though, I am not sure how to do that in an appropriate
> > > manner.  An idea?
> >
> > What does it mean to say that int is unused?  It seems meaningless to me.
>
> OK, in handle_unused_attribute, I prevent 'unused' from applying to
> types, when used in the c+11 attribute syntax.

Well, it does make sense to specify 'unused' in a class definition (i.e. 
when ATTR_FLAG_TYPE_IN_PLACE is set), it just doesn't make sense to 
apply it to a type-specifier.

> +  if (TREE_CODE (*node) != UNION_TYPE)
> +    {
> +      if (flags & ATTR_FLAG_CXX11)
> +       {
> +         /* transparent_union is being used as a c++11 attribute.  In
> +            this mode we force it to apply to a union types only.  */
> +         warning (OPT_Wattributes,
> +                  "attribute %qE applies to union types only", name);
> +         return NULL_TREE;
> +       }
> +    }

I think it would be better to disable looking through the TYPE_DECL in 
this case and then share the warning with the normal case.

> +check_cxx_fundamental_alignment_constraints (tree node,

In this function it would be nice to print out the requested and maximum 
alignments in the error.  And I wonder if we want to offer this as an 
optional warning for GNU attribute syntax.

> +  else if (flags & ATTR_FLAG_CXX11
> +          && DECL_ALIGN (decl) > (1U << i) * BITS_PER_UNIT)
> +    /* C++-11 [dcl.align/4]:
> +       [When multiple alignment-specifiers are specified for an
> +       entity, the alignment requirement shall be set to the
> +       strictest specified alignment].  */
> +    *no_add_attrs = true;

This seems like a good change in general; I'd be inclined to drop the 
check for C++11 syntax.  If there are multiple conflicting alignments 
specified for a declaration, the only things that make sense are to 
choose the largest alignment or give an error; the current behavior of 
choosing the most recently-specified alignment is just broken.

> +      if (cxx11_attribute_p (declspecs->attributes))
> +       warning_at (declspecs->locations[ds_attribute],
> +                   OPT_Wattributes,
> +                   "ignoring attribute applying to missing declarator");

Does this distinguish between attributes before the class-specifier 
(which appertain to any declarations) and attributes after the 
class-specifier (which appertain to the type as used in any declarations)?

I think it would be helpful to clarify that the attributes apply to 
declarators because they're at the beginning of the declaration, and 
keep the inform call for this case as well.

> +         if (std_attrs != NULL_TREE)
> +           {
> +             /*  Attributes should be parsed as part of the the
> +                 declaration, so let's un-parse them.  */
> +             cp_lexer_rollback_tokens (parser->lexer);
> +             std_attrs = NULL_TREE;
> +           }

Huh?  Attributes before a label appertain to the label, not the labeled 
statement.  And wouldn't this cause us to parse the label in an infinite 
loop?

> +            However, in G++ we cannot apply attributes to an already
> +            built *tagged* type, as that breaks some invariants.  So,
> +            for tagged types, to comply with [dcl.spec]/1 which says:
> +
> +                [The attribute-specifier-seq affects the type only for
> +                 the declaration it appears in, not other declarations
> +                 involving the same type]
> +
> +           we are going to apply the attributes to the declaration
> +           that is carrying this type, just like what is done for GNU
> +           attributes.  decl_attributes then knows how to apply the
> +           attributes to the type of that declaration only.  For
> +           non-tagged types, we are going to just apply the
> +           attribute.  */

As discussed above, we don't want to do this.  The attributes appertain 
to the type; for tagged types, that doesn't work, but that just means 
the user's (new) code is wrong, not that we should do something different.

This is especially wrong if the declarator isn't a simple declarator-id.

> +        around as prefix attributes that apply them to the entity

"that apply to"

> +cp_parser_attributes_opt (cp_parser *parser)
> +{
> +

Extra newline.

> +  if (token->type == CPP_NAME)
> +    attr_id = token->u.value;
> +  else if (token->type == CPP_KEYWORD)
> +    attr_id = ridpointers[(int) token->keyword];
> +  else
> +    return NULL_TREE;

Does this handle alternative tokens like "and"?

> +  /* Now parse the optional argument close of the attribute.  */

"clause"

> +      bool complain = !cp_parser_uncommitted_to_tentative_parse_p (parser);

Errors in cxx_alignas_expr don't cause tentative parsing to fail; 
therefore they should not be conditional on tentative parsing.  I don't 
think it's possible for a tentatively parsed alignas to actually be an 
identifier, is it?

>   if (processing_template_decl)
>     {
>       /* If E is a constant, fold it and return it right away.
>          Otherwise, build an ALIGNOF_EXPR that will be substituted
>          into, later at template instantiation time.  */
>       tree cst = TYPE_P (e) ? NULL_TREE : fold_non_dependent_expr_sfinae (e, tf_none);

Shouldn't tf_none be complain here?

>       if (cst != NULL && cst != error_mark_node)
>         return cst;
>       return cxx_alignof_expr (e, complain);
>     }

We shouldn't call cxx_alignof_expr here; the argument to alignas is the 
numerical alignment, whereas cxx_alignof_expr gives us the alignment of 
a value.  And we don't want to call it on types.  I think we can 
actually get away without a template special case here, as long as we 
change the cxx_constant_value lower down to fold_non_dependent_expr_sfinae.

> +  if (TYPE_P (e))
> +    {
> +      e = mark_type_use (e);

mark_type_use is to be used on expressions, not types.

> +  float bar [[gnu::aligned(__alignof__(double))]];

Let's use alignof rather than __alignof__.

> +extern char *f (__const char, int) throw () [[gnu::__pure__]]; // { dg-warning "does not apply to types" }
> +extern int f1 (char *) [[gnu::warn_unused_result]];
> +extern int f3 (char *) [[gnu::nonnull (1)]];
> +  void foo [[gnu::format (printf,2,3)]] (char const * ...);
> +unsigned int f1 [[gnu::const]] ();
> +typedef int (*F) (int) [[gnu::warn_unused_result]];
> +  typedef void (T::*F) (L*) [[gnu::__stdcall__]];
> +int
> +three (void)
> +[[gnu::noreturn]]; // { dg-warning "does not apply to types" }
> +  inline Vector<T> operator<< [[gnu::always_inline]] (int x) const;
> +  [[gnu::fastcall]] void f();
> +extern void foo  () [[gnu::nothrow]]; // { dg-warning "attribute does not apply to types" }

Because of quirks of the internal representation, some of these 
attributes are treated as applying to the decl, and some to the type, 
but we shouldn't be enforcing these arbitrary differences.  I guess we 
can fix this later, but we shouldn't test for the current behavior.

Jason