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

[Martin Sebor <msebor at gmail dot com>]

On 09/21/2016 02:43 PM, Jason Merrill wrote:
> 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.

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).  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?

> > > > +  /* 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

(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.)

> > > > +      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.

I'm happy to start a separate discussion on these two topics unless
you would prefer to.  Please let me know.

Martin