On Tue, Aug 2, 2016 at 5:00 PM, Martin Sebor <mse...@gmail.com> wrote:
>>> struct S1 {
>>> struct S2 { int i, a[]; } s2;
>>> union U { int x; } u;
>>> };
>>>
>>> that need to be treated differently from this one:
>>>
>>> union U1 {
>>> struct S { int i, a[]; } s;
>>> union U2 { int x; } u2;
>>> };
>>
>> Ah, I'm thinking of the following field as u/u2 rather than x. Why
>> does it improve clarity to look inside U/U2 for a following field?
>
> Are you asking if it makes for clearer diagnostics to have AFTER
> point to the instance of the struct in which the overlapping member
> is declared as opposed to the member itself? I.e., instead of the
> note for the second case above:
>
> warning: flexible array member ‘U1::S::a’ belonging to ‘union U1’
> struct S { int i, a[]; } s;
> note: overlaps member ‘int U1::U2::x’ declared here
> union U2 { int x; } u2;
>
> print:
>
> note: overlaps member ‘U1::u2’ declared here
> union U2 { int x; } u2;
> It wasn't a deliberate decision on my part and I don't know how much
> it matters but I've added code to avoid descending into the struct
> when a flexible array member has already been found. It does point
> to the whole struct as I think you were suggesting but it doesn't
> obviate having to look inside nested structs otherwise or storing
> the AFTER member.
Agreed, thanks.
>>> 6.7.2.1, p3:
>>>
>>> A structure or union shall not contain a member with incomplete
>>> or function type [...], except that the last member of a structure
>>> with more than one named member may have incomplete array type; such
>>> a structure (and any union containing, possibly recursively, a member
>>> that is such a structure) shall not be a member of a structure or
>>> an element of an array.
>>
>> Ah, thanks. I note that this says "structure or union" at the
>> beginning of the paragraph but not at the end, which suggests strongly
>> to me that such a structure can be a member of a union.
>
> It would seem reasonable to allow (and the patch still does
> in the very simple cases like the one below) but it's not my
> understanding of the C11 constraints, nor is it apparently how
> writers of other compilers have interpreted them.
>
> Besides C11, I've been trying to make G++ diagnostic-compatible
> with GCC (i.e, issue warnings or errors for the same code). In
> cases where the rules aren't crystal clear I've also been cross-
> checking other compilers (usually Clang and EDG, and sometimes
> also IBM XLC/C++, Intel ICC, and Oracle CC). All but XLC diagnose
> this usage in C:
>
> struct S {
> union U {
> struct X { int i, a[]; } x;
> } u;
> };
>
> Clang, for example, issues the following warning in both C and C++:
>
> 'u' may not be nested in a struct due to flexible array member
> [-Wflexible-array-extensions]
>
> Similarly, Oracle c99 issues warning:
>
> type of struct member "u" can not be derived from structure with
> flexible array member
>
> It could be raised with WG14 for clarification or even proposed
> as a change but given existing practice I don't think it would
> be likely to gain committee support, or worth trying. It seems
> simpler and, IMO, more useful for portability to do what others
> do, at least in the potentially problematic cases like the one
> in c++/71912 where the members overlap.
I definitely agree that following the lead of the C front end is the
right thing to do here. But the C front end doesn't complain about
the definition of union U, only about the use of union U in struct S.
So I still think the first problem mentioned in your comment isn't
actually a problem.
>>>>>>> + /* The type in which an anonymous struct or union containing ARRAY
>>>>>>> + is defined or null if no such anonymous struct or union exists.
>>>>>>> */
>>>>>>> + tree anonctx;
>>>>>>
>>>>>> It seems clearer to find this at diagnostic time by following
>>>>>> TYPE_CONTEXT.
>>>>>
>>>>> I tried this approach and while it's doable (with recursion) I'm
>>>>> not happy with the results. The diagnostics point to places that
>>>>> I think are unclear. For example, given:
>>>>>
>>>>> struct A { int i, a[]; };
>>>>> struct B { long j, b[]; };
>>>>> struct D: A, B { };
>>>>
>>>> I don't see any anonymous aggregates in this example, so how does
>>>> anonctx come into it?
>>>
>>> If there is no anonctx then diagnose_flexrrays will have to figure
>>> out whether the array is a member of an anonymous struct and if so,
>>> find the struct of which it's effectively a member, and otherwise
>>> use the array's immediate context. The above was an example of
>>> the result of a simple implementation of your suggestion.
>>
>> And I don't understand why it would change the output on this testcase.
>
> It doesn't seem to with the latest patch. I don't know which among
> the changes in it has had this effect.
>
>>> As I said, the code could probably be tweaked to produce the same result
>>> as it does now in both cases (anonymous and not), but at the cost
>>> of additional complexity and, IMO, to the detriment of clarity.
>>> What aspect of clarity do you find lacking in the current approach?
>>
>> It just seems unnecessarily complicated. anonctx is set to the
>> innermost non-anonymous-aggregate class containing the flexible array,
>> yes? That is trivially derived from the array decl.
>>
>> Looking at this more closely, it seems that the problem is that
>> "innermost non-anon class" isn't what you want for the example above;
>> for a that is A, and a *is* at the end of A. So it seems that if a
>> were inside an anonymous struct inside A, you would get the same wrong
>> diagnostic output you were seeing with your anon_context function.
>
> After looking into it some more I don't think it's possible to
> determine whether a struct is anonymous from just the declaration
> context of one of its members because of the ANON_AGGR_TYPE_P
> "glitch" I mentioned before (returning true for unnamed structs).
ANON_AGGR_TYPE_P should never be true for an unnamed class that is not
an anonymous aggregate (union or struct). The glitch you mentioned
before was that it is sometimes false for an anonymous struct, which
happens when if we check ANON_AGGR_TYPE_P before we see the ; and so
we don't yet know that it's an anonymous struct, we only know it's
unnamed.
When we're looking at it in the context of the enclosing class,
ANON_AGGR_TYPE_P should always be correct. If it isn't, let's fix
that bug.
> It seems to me that to do that I would need to somehow follow the
> path from an array's DECL_CONTEXT to the DECL_CONTEXT of each of
> the "members" (anonymous structs, not just their types) it's
> recursively defined in. But given the "glitch", how would I tell
> whether the struct is anonymous from just its type? I can't think
> of a way to do that.
You should be able to rely on ANON_AGGR_TYPE_P and use TYPE_CONTEXT.
> I have not removed anonctx because of the problem mentioned above.
> If you're convinced it can be done please show me how.
Does the above help?
>>> It looks like what I actually need here is anon_aggrname_p alone.
>>
>> Why is that better than the anon_aggrname_p call inside
>> TYPE_ANONYMOUS_P? The difference between TYPE_ANONYMOUS_P and
>> anon_aggrname_p (TYPE_IDENTIFIER (t)) is in its handling of
>>
>> typedef struct { ... } name;
>>
>> where TYPE_ANONYMOUS_P is false (because it has a name for linkage
>> purposes) but anon_aggrname_p (TYPE_IDENTIFIER (t)) is true. I don't
>> see why you would care about this distinction. The typedef doesn't
>> declare a member, so it seems uninteresting to check_flexarrays.
>
> They both seem to work for the purposes of the patch. Based on
> what you said it sounds like anon_aggrname_p is more appropriate
> because it answers the question I ask: "is the type unnamed?" and
> not "is the type a class type or an enumerated type or is it
> unnamed?" like TYPE_ANONYMOUS_P, but I'm happy using whichever
> you prefer.
Let's use the macro.
>> Speaking of which, why does your latest patch look at typedefs? What
>> are you missing by looking just at FIELD_DECLs? Even anonymous unions
>> have a FIELD_DECL, though you may need to exempt them from the
>> DECL_ARTIFICIAL check.
>
> It looks at typedefs in order to detect the problem in unnamed
> structs like the one below:
>
> struct A { typedef struct { int a[]; } B; };
>
> The unnamed struct is skipped in check_flexarrays (because it could
> be anonymous) and so it's handled when struct A is.
Hmm, doesn't that mean that
typedef struct { int a[]; } B;
is never handled? Perhaps you want to do this check from
cp_parser_simple_declaration, when we know whether or not there's a
declarator?
>> Do you have ideas about how to improve the naming? Perhaps change
>> TYPE_ANONYMOUS_P to TYPE_NO_LINKAGE_NAME?
>
> I haven't thought about changing names but TYPE_NO_LINKAGE_NAME
> seems better than TYPE_ANONYMOUS_P.
Or perhaps TYPE_UNNAMED_P.
> What I think might help in
> general is a set of APIs whose names match well-known concepts
> in the language. As another example, besides TYPE_ANONYMOUS_P
> it took me a while to grok the subtleties of macros like
> DECL_ARTIFICIAL, DECL_SELF_REFERENCE_P, and especially
> DECL_IMPLICIT_TYPEDEF_P when figuring out if a typedef is
> an alias for an unnamed struct.
I suppose DECL_SELF_REFERENCE_P could be renamed to
DECL_INJECTED_CLASS_NAME_P, but that's what it means. I'll add that
term to the comment.
> I had hoped for a more straightforward way than this:
>
> /* Is FLD a typedef for an anonymous struct? */
> bool anon_type_p
> = (TREE_CODE (fld) == TYPE_DECL
> && DECL_IMPLICIT_TYPEDEF_P (fld)
> && anon_aggrname_p (DECL_NAME (fld)));
I would suggest looking for the FIELD_DECL for the anonymous aggregate
instead, i.e.
TREE_CODE (fld) == FIELD_DECL && ANON_AGGR_TYPE_P (TREE_TYPE (fld))
> I realize that the front end's representation of the program is
> populated piece by piece and that not all the pieces are in place
> at every point, so maybe it's just a matter of a learning curve.
> It just seems more steep than it should be.
Sure. Ideas about improvement are welcome.
Jason
