Re: [V3][PATCH 1/3] Provide counted_by attribute to flexible array member field (PR108896)

[Qing Zhao <qing.zhao@oracle.com>]

> On Oct 18, 2023, at 11:18 AM, Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
> 
> On 2023-10-18 10:51, Qing Zhao wrote:
>>>>> +   member FIELD_DECL is a valid field of the containing structure's fieldlist,
>>>>> +   FIELDLIST, Report error and remove this attribute when it's not.  */
>>>>> +static void
>>>>> +verify_counted_by_attribute (tree fieldlist, tree field_decl)
>>>>> +{
>>>>> +  tree attr_counted_by = lookup_attribute ("counted_by",
>>>>> +                       DECL_ATTRIBUTES (field_decl));
>>>>> +
>>>>> +  if (!attr_counted_by)
>>>>> +    return;
>>>>> +
>>>>> +  /* If there is an counted_by attribute attached to the field,
>>>>> +     verify it.  */
>>>>> +
>>>>> +  const char *fieldname
>>>>> +    = IDENTIFIER_POINTER (TREE_VALUE (TREE_VALUE (attr_counted_by)));
>>>>> +
>>>>> +  /* Verify the argument of the attrbute is a valid field of the
>>>> s/attrbute/attribute/
>>>>> +     containing structure.  */
>>>>> +
>>>>> +  tree counted_by_field = get_named_field (fieldlist, fieldname);
>>>>> +
>>>>> +  /* Error when the field is not found in the containing structure.  */
>>>>> +  if (!counted_by_field)
>>>>> +    {
>>>>> +      error_at (DECL_SOURCE_LOCATION (field_decl),
>>>>> +        "%qE attribute argument not a field declaration"
>>>>> +        " in the same structure, ignore it",
>>>>> +        (get_attribute_name (attr_counted_by)));
>>>> Probably someone with English as a first language would make a better suggestion, but how about:
>>>>   Argument specified in %qE attribute is not a field declaration in the
>>>>   same structure, ignoring it.
>>>>> +
>>>>> +      DECL_ATTRIBUTES (field_decl)
>>>>> +    = remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl));
>>>>> +    }
>>>>> +  else
>>>>> +  /* Error when the field is not with an integer type.  */
>>>> Suggest: Flag an error when the field is not of an integer type.
>>>>> +    {
>>>>> +      while (TREE_CHAIN (counted_by_field))
>>>>> +    counted_by_field = TREE_CHAIN (counted_by_field);
>>>>> +      tree real_field = TREE_VALUE (counted_by_field);
>>>>> +
>>>>> +      if (TREE_CODE (TREE_TYPE (real_field)) != INTEGER_TYPE)
>>>>> +    {
>>>>> +      error_at (DECL_SOURCE_LOCATION (field_decl),
>>>>> +             "%qE attribute argument not a field declaration"
>>>>> +             " with integer type, ignore it",
>>>>> +             (get_attribute_name (attr_counted_by)));
>>>> Suggest:
>>>>   Argument specified in %qE attribute is not of an integer type,
>>>>   ignoring it.
>>>>> +
>>>>> +      DECL_ATTRIBUTES (field_decl)
>>>>> +        = remove_attribute ("counted_by", DECL_ATTRIBUTES (field_decl));
>>>>> +    }
>>>>> +    }
>>>>> +
>>>>> +  return;
>>> 
>>> I forgot to mention the redundant return here.
>> Could you please clarify a little bit here, why the return here is redundant?
> 
> It's the last line in the function, so even without that statement the function will return.
Oh, I see. -:)
Actually,I always put an explicit return  there even though it’s the last line and return implicitly. 

Qing

> 
> Thanks,
> Sid