cor3ntin added inline comments.
================ Comment at: clang/test/SemaCXX/offsetof.cpp:106 +int x3[__builtin_offsetof(struct X2, X2::static_a) == 0 ? 1 : -1]; // expected-error{{no member named 'static_a'}} +int x4[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // expected-error{{no member named 'X2'}} + ---------------- yichi170 wrote: > cor3ntin wrote: > > yichi170 wrote: > > > hubert.reinterpretcast wrote: > > > > aaron.ballman wrote: > > > > > yichi170 wrote: > > > > > > aaron.ballman wrote: > > > > > > > There's one more test I'd like to see: > > > > > > > ``` > > > > > > > struct S { > > > > > > > int Foo; > > > > > > > }; > > > > > > > > > > > > > > template <typename Ty> > > > > > > > void func() { > > > > > > > static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, ""); > > > > > > > } > > > > > > > > > > > > > > void inst() { > > > > > > > func<S>(); > > > > > > > } > > > > > > > ``` > > > > > > It would get the compile error in the current patch, but I think it > > > > > > should be compiled without any error, right? > > > > > Correct, that should be accepted: https://godbolt.org/z/1f6a9Yaxa > > > > Should expect this to pass too: > > > > ``` > > > > template <typename T> > > > > struct Z { > > > > static_assert(!__builtin_offsetof(T, template Q<T>::x)); > > > > }; > > > > > > > > struct A { > > > > template <typename T> using Q = T; > > > > int x; > > > > }; > > > > > > > > Z<A> za; > > > > ``` > > > Wow. Does it mean we cannot simply parse the identifier, `::`, `.` and > > > brackets in `__builtin_offsetof`? > > GCC seems to support that. > > > > We probably want to call `ParseOptionalCXXScopeSpecifier` and store the > > `NestedNameSpecifierLoc` we'd get from it (and then parse the (sequence of) > > identifier(s) corresponding to the member) as we do now. > > > > The documentation of > > https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/Offsetof.html#index-_005f_005fbuiltin_005foffsetof > > > > seems inaccurate, > > > > it seems to be > > > > `"__builtin_offsetof" "(" typename "," nested-name-specifier > > offsetof_member_designator ")"` > > > > > > Note that you will have to take care of transforming the nested name in > > TreeTransform and handle type dependencies. Let me know if you have further > > questions, > > it's more involved than what you signed for. Sorry for not spotting that > > earlier (Thanks @hubert.reinterpretcast !) > Thank you for all the help! I'll take a look at it! I was wrong, we need another approach. I think the grammar is actually ``` member-designator: qualified-id member-designator.qualified-id member-designator.qualified-id ``` IE, we should support https://godbolt.org/z/eEq8snMc8 Unfortunately, this looks like a much bigger change that we envisioned when we tagged this as a good first issue, to the extent I'm not sure what is actually the right design is would be. For each component I imagine we want to store `NestedNameSpecifierLoc + DeclarationNameInfo` The parser would have to produce a CXXScopeSpec + UnqualifiedId pair for each component. The expression is dependent if any of the component is type dependent, `OffsetOfNode` would have to change, but i think we can get away Only changing the identifier case (ie the dependent case) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157201/new/ https://reviews.llvm.org/D157201 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits