On Thu, Sep 22, 2016 at 7:39 PM, Martin Sebor <mse...@gmail.com> wrote:
> On 09/21/2016 02:43 PM, Jason Merrill wrote:
>> 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.
>
> I suspect I'm simply looking at it from a different point of view.
> The definition
>
> tree fldtype = TREE_CODE (fld) == FIELD_DECL ? TREE_TYPE (fld) : fld
>
> denotes the type of the field if fld is a data member. Otherwise,
> it denotes a type (like a typedef).
FLD is always a _DECL. The result of this assignment is that fldtype
refers to either a _TYPE or a _DECL. This creates a strange
asymmetry, and none of the following code seems to depend on it.
> Fldtype seems as a good a name
> as any but I'll gladly rename it to something else if that lets us
> move forward. What would you prefer?
I would prefer
tree fldtype = TREE_TYPE (fld);
so that it's always 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 { }?
>
> Sorry, I don't understand the question. A flexible array is always
> "wrapped in struct { }" so I'm not sure what you mean by that. And
> "we want to talk about D" because that's where the next member after
> a is defined (we could also say it's defined in struct Y and I think
> I may have been down that path at one point and decided this was fine
> or perhaps even better or simpler but I don't recall which for sure.)
>
> Btw., I used quotes above only to explain how I read your question,
> not to mock what you said in any way.
>
> Going back and looking at some of the older patches, this hunk above
> was changed from the original patch like so:
>
> @@ -6923,7 +6930,10 @@ diagnose_flexarrays (tree t, const flexmems_t *fmem)
> /* 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 = fmem->anonctx ? fmem->anonctx : t;
> + tree fmemctx = anon_context (fmem->array);
> + bool anon_p = fmemctx != DECL_CONTEXT (fmem->array);
> + if (!anon_p)
> + fmemctx = t;
>
> I made this change because you preferred deriving the fmemctx value
> in a new function rather than storing it the fmem->anonctx member.
> I don't know if this helps clarify it.
>
> https://gcc.gnu.org/ml/gcc-patches/2016-08/msg00153.html
My point is that for
struct X { int n, a[]; };
struct X2 { struct { int n, a[]; }; };
struct Y { int n, b[]; };
struct D: X, Y { };
struct D2: X2, Y { };
We say
wa3.C:1:21: error: flexible array member ‘X::a’ not at end of ‘struct D’
wa3.C:2:31: error: flexible array member ‘X2::<unnamed struct>::a’ not
at end of ‘struct X2’
If we want to say "struct D", why don't we also want to say "struct
D2"? It seems that you could unconditionally set fmemctx to t and not
bother calling anon_context.
> (FWIW, it's been a stressful couple of days with the regressions
> that my recent commit caused, so I hope you can bear with me if
> I seem slow or dense on these two points.)
No worries.
> [comments with parameter names/default arguments]
> I'm happy to start a separate discussion on these two topics unless
> you would prefer to. Please let me know.
Please do.
Jason
