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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:279-280
   ``%hd``.
+- Reject type definitions in the ``type`` argument of ``__builtin_offsetof`` 
+  according to `WG14 N2350 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm>`_.
 
----------------
You should move this one down to the C2x Feature Support section instead, since it's a C2x paper.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1650
+def err_type_defined_in_offsetof : Error<
+  "%0 cannot be defined in '__builtin_offsetof'">;
 
----------------
We might want this change, we might not -- can you test the diagnostic behavior when using `#include <stddef.h>`? Does it print `__builtin_offsetof` in the following example?
```
#include <stddef.h>

int main() {
  return offsetof(struct S { int a; }, a);
}
```
when executed with `clang -fsyntax-only -ffreestanding -std=c2x test.c`

If it prints the builtin name, I think we'll want to look at the builtin token to see if it was expanded from a macro named `offsetof` to improve the diagnostic quality.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:17034
+    Diag(New->getLocation(), diag::err_type_defined_in_offsetof)
+        << Context.getTagDeclType(New);
+    Invalid = true;
----------------
You should be able to pass in the `TagDecl` directly because the diagnostics engine knows how to print a `NamedDecl`.


================
Comment at: clang/test/Sema/offsetof.c:73-75
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
----------------
Can you move this into a test named `clang/test/C/C2x/n2350.c` which looks like the other tests in that directory? That's the testing directory we're putting some basic validation tests for C papers we've implemented so that we can better track what features we claim to support.


================
Comment at: clang/test/SemaCXX/offsetof.cpp:93
+    int a;
+    struct B
+    {
----------------
I'd like a FIXME comment here about wanting to diagnose this someday.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133574