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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
