Hi, Joseph,
Thanks a lot for the reviews.
> On Mar 25, 2024, at 16:44, Joseph Myers <josmy...@redhat.com> wrote:
>
> On Wed, 20 Mar 2024, Qing Zhao wrote:
>
>> + /* This attribute only applies to a C99 flexible array member type. */
>> + else if (! c_flexible_array_member_type_p (TREE_TYPE (decl)))
>> + {
>> + error_at (DECL_SOURCE_LOCATION (decl),
>> + "%qE attribute is not allowed for a non"
>> + " flexible array member field",
>
> "non-flexible" not "non flexible" ("non" shouldn't appear as a word on its
> own).
Okay.
>
>> + /* Error when the field is not found in the containing structure. */
>> + if (!counted_by_field)
>> + error_at (DECL_SOURCE_LOCATION (field_decl),
>> + "Argument %qE to the %qE attribute is not a field declaration"
>> + " in the same structure as %qD", fieldname,
>
> Diagnostics should start with a lowercase letter, "argument" not
> "Argument”.
Okay.
>
>> + if (TREE_CODE (TREE_TYPE (real_field)) != INTEGER_TYPE)
>> + error_at (DECL_SOURCE_LOCATION (field_decl),
>> + "Argument %qE to the %qE attribute is not a field declaration"
>> + " with an integer type", fieldname,
>
> Likewise.
Okay.
>
> Generally checks for integer types should allow any INTEGRAL_TYPE_P,
> rather than just INTEGER_TYPE. For example, it should be valid to use
> this attribute with a field with _BitInt type. (It would be fairly
> useless with a _BitInt larger than size_t, but maybe e.g. someone knows
> the size in their code must fit into 24 bits and so uses unsigned
> _BitInt(24) for the field.)
Okay. Will change this.
>
> Of course there should be corresponding testcases for _Bool / enum /
> _BitInt count fields.
And add corresponding testing cases.
>
> What happens when there are multiple counted_by attributes on the same
> field? As far as I can see, all but one end up being ignored (by the code
> that actually uses the attribute).
In general, is there any rule for handling multiple same attributes in GCC?
i.e, from left to right, the last one wins? Or something else? I’d like to
following the consistent rule with other places in GCC.
> I think multiple such attributes using
> different identifiers should be diagnosed, even if all the identifiers are
> indeed integer fields in the same structure - it doesn't seem meaningful
> to say that multiple fields give the count of elements.
Yes, this is reasonable. Shall we ignore all but the last one? And issue
warnings at the same time?
i.e. for the following:
struct trailing_array {
int c1;
int final;
int array_4[] __attribute ((counted_by (c1))) __attribute ((counted_by
(final)));
};
For the above, issue warning by default:
multiple 'counted-by' attribute specified for the same flexible array member
field “array_4”, only the last one “final” is valid.
??
> (Multiple
> attributes with the *same* identifier are probably OK to allow; maybe that
> could arise in code using complicated macros that end up adding the
> attribute more than once.)
Okay.
Shall we issue warnings for this case? (Probably not??)
>
>> +@cindex @code{counted_by} variable attribute
>> +@item counted_by (@var{count})
>> +The @code{counted_by} attribute may be attached to the C99 flexible array
>> +member of a structure. It indicates that the number of the elements of the
>> +array is given by the field named "@var{count}" in the same structure as the
>> +flexible array member.
>
> You shouldn't use ASCII quotes like that in Texinfo (outside @code etc.
> where they represent literal quotes in programming language source code).
> You can say ``@var{count}'' if you wish to quote the name.
A little confused with the above..
So, what should I change in the above statement?
>
>> +The field that represents the number of the elements should have an
>> +integer type. Otherwise, the compiler will report a warning and ignore
>> +the attribute.
>> +When the field that represents the number of the elements is assigned a
>> +negative integer value, the compiler will treat the value as zero.
>
> In general it's best for documentation to be in the present tense (so the
> compiler *reports* a warning rather than "will report", *treats* the value
> as zero rather than "will treat").
thanks, will update them.
>
>> +It's the user's responsibility to make sure the above requirements to
>> +be kept all the time. Otherwise the compiler will report warnings,
>> +at the same time, the results of the array bound sanitizer and the
>> +@code{__builtin_dynamic_object_size} is undefined.
>
> Likewise.
Okay.
Thanks a lot.
Qing
>
> --
> Joseph S. Myers
> josmy...@redhat.com
>
