[PATCH] D133574: [C2x] reject type definitions in offsetof

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D133574#4047677 <https://reviews.llvm.org/D133574#4047677>, @inclyc wrote:

> ping :)

Oh! I missed your pings because this was already marked as accepted, I'm sorry for the delay in answering your question.

In D133574#3994050 <https://reviews.llvm.org/D133574#3994050>, @inclyc wrote:

> Hi @aaron.ballman! seems we are failing tests on recent change 2fc5a3410087c209567c7e4a2c457b6896c127a3 <https://reviews.llvm.org/rG2fc5a3410087c209567c7e4a2c457b6896c127a3>
>
>   /* The DR asked a question about whether defining a new type within offsetof
>    * is allowed. C2x N2350 made this explicitly undefined behavior, but Clang
>    * has always supported defining a type in this location, and GCC also
>    * supports it.
>    */
>    (void)__builtin_offsetof(struct S { int a; }, a);
>
> Do you think this patch should still be merged into mainline? Or do we turn this error into a warning?

We don't currently document `__builtin_offsetof` and GCC's documentation (https://gcc.gnu.org/onlinedocs/gcc/Offsetof.html) does not explicitly claim they support this behavior, so I'm fine with diagnosing this UB when we previously didn't. If we find this breaks significant code in practice, maybe we'll come to another decision, but for now I'd say you should update that test to expect the error (and update the comment as well).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133574/new/

https://reviews.llvm.org/D133574