On Tue, Sep 20, 2016 at 1:01 PM, Martin Sebor <mse...@gmail.com> wrote:
> On 09/16/2016 12:19 PM, Jason Merrill wrote:
>> On 09/14/2016 01:03 PM, Martin Sebor wrote:
>>>
>>> + /* Type of the member. */
>>> + tree fldtype = TREE_CODE (fld) == FIELD_DECL ? TREE_TYPE (fld)
>>> : fld;
>>
>> Why set "fldtype" to be a TYPE_DECL rather than its type?
>
> I'm not sure I understand the question but (IIRC) the purpose of
> this code is to detect invalid uses of flexible array members in
> typedefs such as this:
>
> struct S { typedef struct { int i, a[]; } X2 [2]; };
>
> Sadly, it doesn't do anything for
>
> struct X { int i, a[]; };
> struct S { typedef struct X X2 [2]; };
The issue is I don't see anything that uses fldtype as a decl, and
it's strange to have a variable called "*type" that can either be a
type or a decl, which later code still assumes will be a type.
>>> + /* The context of the flexible array member. Either the struct
>>> + in which it's declared or, for anonymous structs and unions,
>>> + the struct/union of which the array is effectively a member. */
>>> + tree fmemctx = anon_context (fmem->array);
>>> + bool anon_p = fmemctx != DECL_CONTEXT (fmem->array);
>>> + if (!anon_p)
>>> + fmemctx = t;
>>
>> Why do you need to do something different here based on anon_p? I don't
>> see why that would affect whether we want to look through intermediate
>> non-anonymous classes.
>
> In code like this:
>
> struct X { int n, a[]; };
> struct Y { int n, b[]; };
>
> struct D: X, Y { };
>
> The test above make the diagnostic point to the context of the invalid
> flexible array member, or Y::b, rather than that of X. Without it, we
> end up with:
>
> error: flexible array member ‘X::a’ not at end of ‘struct X’
>
> rather than with
>
> error: flexible array member ‘X::a’ not at end of ‘struct D’
Yes, but why does whether we want to talk about X or D change if a is
wrapped in struct { }?
>>> + check_flexarrays (basetype, fmem, true);
>>
>> Please decorate boolean literal arguments like this with the name of the
>> parameter, e.g. /*base_p*/true.
>
> Sure. I should mention that Jeff recently advised against it in
> another review, so it would be nice to adopt the same convention
> throughout and document it (I'm happy to add it to the Wiki once
> it has been agreed on):
>
> https://gcc.gnu.org/ml/gcc-patches/2016-08/msg00354.html
Interesting. It's pretty common in the C++ front end.
> FWIW, I haven't found mentioning the formal argument in a comment
> a useful or consistent convention. Other arguments' names aren't
> mentioned, and the bool argument's name cannot be mentioned when
> it has a default value.
True, but other arguments usually have more implied meaning and so I
think they are less of a barrier to reading.
> On the other hand, a convention I do find useful (though not one
> that seems to be used in GCC) is indicating in a comment in the
> definition of a function the value of the default argument in
> functions declared to take one.
I agree that would be a good convention.
Jason
