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

[Jason Merrill]

On 09/17/2012 11:35 AM, Dodji Seketeli wrote:
>> >And I wonder if we want to offer this as an optional warning for GNU
>    attribute syntax.
>
> What option would be used to control this optional feature?  Would
> you accept this a separate patch?

Let's not worry about this for now.

> +	  found_attrs = true;

You don't need a separate found_attrs variable; you can just check 
whether attrs is set or not.  VEC_iterate sets the element pointer to 
NULL at the end of the vector.  You should also be able to share more of 
the code between the cases where you do or do not already have the 
appropriate namespace set up.

> +static scoped_attributes_t*
> +find_attribute_namespace (const char* ns)
> +{
> +  unsigned ix;
> +  scoped_attributes_t *iter;
> +
> +  FOR_EACH_VEC_ELT (scoped_attributes_t, attributes_table, ix, iter)
> +    if (ns == iter->ns
> +	|| (iter->ns != NULL
> +	    && ns != NULL
> +	    && !strcmp (iter->ns, ns)))
> +      {
> +	if (iter->attribute_hash == 0)
> +	  iter->attribute_hash =
> +	    htab_create (200, hash_attr, eq_attr, NULL);
> +	return VEC_index (scoped_attributes_t, attributes_table, ix);
> +      }

Rather than write the code to search the table in two places, let's call 
this function from register_scoped_attributes.

You don't need to do another VEC_index since you already have the 
pointer you want in 'iter'.

> +attributes_array_length (const struct attribute_spec *attributes_array)

Rather than calculate this first and then pass it to 
register_scoped_attributes, why not just have register_scoped_attributes 
stop at a null entry?

> +  if (name_space->attribute_hash == NULL)
> +   name_space->attribute_hash = htab_create (200, hash_attr, eq_attr, NULL);

Why do we need this both here and in find_attribute_namespace?

>  }
> +/*
> +   If in c++-11, check if the c++-11 alignment constraint with respect

Need a blank line after the }.

>>> +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.
>
> Done.

I don't see that:

> +  if (alignment_too_large_p)
> +    error ("requested alignment is too large");

>> 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.
>
> Done.  I realized that I forgot to check for DECL_USER_ALIGNED there.
> Fixed too.

I don't see this either:

> +  else if (flags & ATTR_FLAG_CXX11
> +	   && DECL_ALIGN (decl) > (1U << i) * BITS_PER_UNIT)

Did you send me an old patch by mistake?

Jason