Endill 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: > > > 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) > > > Would it be better for me to close this patch and submit a new one if I find > out how to implement it? I hope others won't hesitate to contribute because > I'm working on this. I don't want to cause any delays in the release plan! > Would it be better for me to close this patch and submit a new one if I find > out how to implement it? A possible approach is to follow the example of member reference expression in its dot form. > I hope others won't hesitate to contribute because I'm working on this. I > don't want to cause any delays in the release plan! No worries, 17 release has branched a month ago, so you don't have to feel stressed over that. 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