Re: [PATCH] accept flexible arrays in struct in unions (c++/71912 - [6/7 regression])

[Jason Merrill <jason at redhat dot com>]

On Tue, Sep 20, 2016 at 1:01 PM, Martin Sebor <msebor@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