jyknight added inline comments.
================
Comment at: clang/lib/CodeGen/CGExpr.cpp:906
// member, only a T[0] or T[] member gets that treatment.
+ // Under StrictFlexArraysLevel, obey c99+ that disallows FAM in union,
see
+ // C11 6.7.2.1 ยง18
----------------
serge-sans-paille wrote:
> jyknight wrote:
> > I believe this bit is incorrect -- it should just go back to 'return
> > true;'. The StrictFlexArraysLevel check above already eliminates the cases
> > we want to eliminate (size==1 in strictness-level 2.)
> Well, if we are in strictness-level 2, with an undefined size or size = 0, we
> can still reach that path, and don't want to return 'true' because FAM in
> union are in invalid per the standard.
Yes, we can reach this path, which is why the change is incorrect. We should
not be changing the FAMness of undefined size, or size == 0, in any of the
modes. To be more specific --
`union X { int x[0]; };` should still be a FAM in all strictness modes. (if you
don't want zero-length-arrays, use `-Werror=zero-length-array`).
For `union X { int x[]; };`: this ought to be a compiler error. It's likely
just an oversight that we currently accept it; I'd be OK with a (separate)
patch to fix that. (GCC emits an error, so there's unlikely to be compatibility
issues with such a change.)
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15972
+ // FIXME: this check should belong to the IsTailPaddedMemberArray call
+ // below.
----------------
What I meant to say is that both the IsTailPaddedMemberArray call, and this
code, should be moved up above, in order to set IsUnboundedArray to true, so we
will get the checks from that block.
(But doesn't need to be in this patch, fine to just leave a comment to that
effect).
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15804
+
+ // FIXME: we should also allow Size = 0 here per the definition of
+ // StrictFlexArraysLevel, but that's backward incompatible with previous
clang
----------------
jyknight wrote:
> serge-sans-paille wrote:
> > jyknight wrote:
> > > Presumably the size-zero/unsized cases are already being taken care of
> > > elsewhere in the code? I find it hard to believe we are currently
> > > emitting diagnostics for size-0 FAM which we don't emit for size-1 FAM?
> > correct
> The FIXME comment isn't correct, since only nonzero sizes ever reach this
> function. The code can be simplified too. Also -- there is another FIXME that
> should be here, regarding the behavior of size>1 FAMs.
>
> I suggest (with rewrapped comment of course):
> ```
> if (!ND) return false;
> // FIXME: While the default -fstrict-flex-arrays=0 permits Size>1 trailing
> arrays to be treated as flexible-array-members, we still emit diagnostics as
> if they are not. Pending further discussion...
> if (StrictFlexArraysLevel >= 2 || Size != 1) return false;`
> ```
>
You didn't like the code change I suggested there?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:15969
// access which precedes the array bounds.
if (BaseType->isIncompleteType())
return;
----------------
jyknight wrote:
> serge-sans-paille wrote:
> > And here
> Looks like actually the `int x[]` case is handled with the large
> "IsUnboundedArray" condition above not here...
>
> And, actually, all of that code to generate warnings for
> larger-than-addrspace offsets OUGHT to be getting used for `int x[0]` and
> `int x[1]` flexible arrays, too. Needs another FIXME for that...
Is it redundant? This check is for an _incomplete_ type as the array element
type, which doesn't seem directly related to unbounded array sizes. (though it
also prevents knowing the size of the array)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126864/new/
https://reviews.llvm.org/D126864
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
