[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Vlad Serebrennikov via Phabricator via cfe-commits]

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 
> > > > > > > > > void func() {
> > > > > > > > >   static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, "");
> > > > > > > > > }
> > > > > > > > > 
> > > > > > > > > void inst() {
> > > > > > > > >   func();
> > > > > > > > > }
> > > > > > > > > ```
> > > > > > > > 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 
> > > > > > struct Z {
> > > > > >   static_assert(!__builtin_offsetof(T, template Q::x));
> > > > > > };
> > > > > > 
> > > > > > struct A {
> > > > > >   template  using Q = T;
> > > > > >   int x;
> > > > > > };
> > > > > > 
> > > > > > Z 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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Yichi Lee via Phabricator via cfe-commits]

yichi170 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'}}
+

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 
> > > > > > > > void func() {
> > > > > > > >   static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, "");
> > > > > > > > }
> > > > > > > > 
> > > > > > > > void inst() {
> > > > > > > >   func();
> > > > > > > > }
> > > > > > > > ```
> > > > > > > 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 
> > > > > struct Z {
> > > > >   static_assert(!__builtin_offsetof(T, template Q::x));
> > > > > };
> > > > > 
> > > > > struct A {
> > > > >   template  using Q = T;
> > > > >   int x;
> > > > > };
> > > > > 
> > > > > Z 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!


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Corentin Jabot via Phabricator via cfe-commits]

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 
> > > > > > > void func() {
> > > > > > >   static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, "");
> > > > > > > }
> > > > > > > 
> > > > > > > void inst() {
> > > > > > >   func();
> > > > > > > }
> > > > > > > ```
> > > > > > 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 
> > > > struct Z {
> > > >   static_assert(!__builtin_offsetof(T, template Q::x));
> > > > };
> > > > 
> > > > struct A {
> > > >   template  using Q = T;
> > > >   int x;
> > > > };
> > > > 
> > > > Z 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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Yichi Lee via Phabricator via cfe-commits]

yichi170 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'}}
+

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 
> > > > > > void func() {
> > > > > >   static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, "");
> > > > > > }
> > > > > > 
> > > > > > void inst() {
> > > > > >   func();
> > > > > > }
> > > > > > ```
> > > > > 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 
> > > struct Z {
> > >   static_assert(!__builtin_offsetof(T, template Q::x));
> > > };
> > > 
> > > struct A {
> > >   template  using Q = T;
> > >   int x;
> > > };
> > > 
> > > Z 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!


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Corentin Jabot via Phabricator via cfe-commits]

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:
> 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 
> > > > > void func() {
> > > > >   static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, "");
> > > > > }
> > > > > 
> > > > > void inst() {
> > > > >   func();
> > > > > }
> > > > > ```
> > > > 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 
> > struct Z {
> >   static_assert(!__builtin_offsetof(T, template Q::x));
> > };
> > 
> > struct A {
> >   template  using Q = T;
> >   int x;
> > };
> > 
> > Z 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 !)


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Yichi Lee via Phabricator via cfe-commits]

yichi170 added a comment.

Is there any code handling the nested qualifier that I can reference? I tried 
to modify the `OffsetOfNode`, which allows me to handle the basic template case 
but still failed on the nested 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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Yichi Lee via Phabricator via cfe-commits]

yichi170 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'}}
+

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 
> > > > void func() {
> > > >   static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, "");
> > > > }
> > > > 
> > > > void inst() {
> > > >   func();
> > > > }
> > > > ```
> > > 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 
> struct Z {
>   static_assert(!__builtin_offsetof(T, template Q::x));
> };
> 
> struct A {
>   template  using Q = T;
>   int x;
> };
> 
> Z za;
> ```
Wow. Does it mean we cannot simply parse the identifier, `::`, `.` and brackets 
in `__builtin_offsetof`?


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast 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'}}
+

aaron.ballman wrote:
> yichi170 wrote:
> > aaron.ballman wrote:
> > > There's one more test I'd like to see:
> > > ```
> > > struct S {
> > >   int Foo;
> > > };
> > > 
> > > template 
> > > void func() {
> > >   static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, "");
> > > }
> > > 
> > > void inst() {
> > >   func();
> > > }
> > > ```
> > 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 
struct Z {
  static_assert(!__builtin_offsetof(T, template Q::x));
};

struct A {
  template  using Q = T;
  int x;
};

Z za;
```


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/test/SemaCXX/offsetof.cpp:104
+struct X2 { int a; static int static_a; };
+int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1];
+int x3[__builtin_offsetof(struct X2, X2::static_a) == 0 ? 1 : -1]; // 
expected-error{{no member named 'static_a'}}

Tests should include cases where the member is a direct member of a base class 
or an anonymous union therein (and named variously by the qualification with 
the base class name, the complete class name, an intermediate base class name, 
the base class name qualified with an intermediate base class, the base class 
name denoted using `::identity_t`, etc.).

Additionally, there should be a diagnostic test case where the qualifier is for 
an unrelated identically-defined class in another namespace.


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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:
> aaron.ballman wrote:
> > There's one more test I'd like to see:
> > ```
> > struct S {
> >   int Foo;
> > };
> > 
> > template 
> > void func() {
> >   static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, "");
> > }
> > 
> > void inst() {
> >   func();
> > }
> > ```
> 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


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Yichi Lee via Phabricator via cfe-commits]

yichi170 marked an inline comment as done.
yichi170 added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:59
 
+- Improved ``__builtin_offsetof`` support, allowing qualified name in member 
designator. This fixes [Issue 
64154](https://github.com/llvm/llvm-project/issues/64154).
 

aaron.ballman wrote:
> 
> 





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'}}
+

aaron.ballman wrote:
> There's one more test I'd like to see:
> ```
> struct S {
>   int Foo;
> };
> 
> template 
> void func() {
>   static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, "");
> }
> 
> void inst() {
>   func();
> }
> ```
It would get the compile error in the current patch, but I think it should be 
compiled without any error, right?


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:59
 
+- Improved ``__builtin_offsetof`` support, allowing qualified name in member 
designator. This fixes [Issue 
64154](https://github.com/llvm/llvm-project/issues/64154).
 





Comment at: clang/lib/Parse/ParseExpr.cpp:2646-2651
+if (Tok.is(tok::coloncolon) && getLangOpts().CPlusPlus) {
+  Comps.back().Kind = Sema::OffsetOfComponent::Qualifier;
+} else if (Tok.is(tok::coloncolon) && !getLangOpts().CPlusPlus) {
+  Res = ExprError();
+  break;
+}

I think this is slightly more clear.



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'}}
+

There's one more test I'd like to see:
```
struct S {
  int Foo;
};

template 
void func() {
  static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, "");
}

void inst() {
  func();
}
```


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:6038
 SourceLocation LocStart, LocEnd;
 bool isBrackets;  // true if [expr], false if .ident
+bool isQualifier;

aaron.ballman wrote:
> I think we should combine `isBrackets` and `isQualifier` since a component 
> can't be both at the same time anyway. e.g.,
> ```
> enum {
>   Brackets,   // U.E is valid
>   Identifier, // U.IdentInfo is valid
>   Qualifier,  // Nothing in U is valid
> } Kind;
> ```
This are not the best names.

ArrayElement, Subobject, NestedNameQualifier

Are probably more descriptive.


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Yichi Lee via Phabricator via cfe-commits]

yichi170 updated this revision to Diff 548632.
yichi170 marked 4 inline comments as done.
yichi170 added a comment.

Applied the suggestions. Thanks for giving me feedback!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157201

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaCXX/offsetof.cpp

Index: clang/test/SemaCXX/offsetof.cpp
===
--- clang/test/SemaCXX/offsetof.cpp
+++ clang/test/SemaCXX/offsetof.cpp
@@ -98,3 +98,10 @@
 B x;
   }, a);
 }
+
+// https://github.com/llvm/llvm-project/issues/64154
+struct X2 { int a; static int static_a; };
+int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1];
+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'}}
+
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -11028,7 +11028,7 @@
   for (unsigned I = 0, N = E->getNumComponents(); I != N; ++I) {
 const OffsetOfNode  = E->getComponent(I);
 Component Comp;
-Comp.isBrackets = true;
+Comp.Kind = Sema::OffsetOfComponent::Brackets;
 Comp.LocStart = ON.getSourceRange().getBegin();
 Comp.LocEnd = ON.getSourceRange().getEnd();
 switch (ON.getKind()) {
@@ -11039,14 +11039,14 @@
 return ExprError();
 
   ExprChanged = ExprChanged || Index.get() != FromIndex;
-  Comp.isBrackets = true;
+  Comp.Kind = Sema::OffsetOfComponent::Brackets;
   Comp.U.E = Index.get();
   break;
 }
 
 case OffsetOfNode::Field:
 case OffsetOfNode::Identifier:
-  Comp.isBrackets = false;
+  Comp.Kind = Sema::OffsetOfComponent::Identifier;
   Comp.U.IdentInfo = ON.getFieldName();
   if (!Comp.U.IdentInfo)
 continue;
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16612,7 +16612,7 @@
   SmallVector Comps;
   SmallVector Exprs;
   for (const OffsetOfComponent  : Components) {
-if (OC.isBrackets) {
+if (OC.Kind == OffsetOfComponent::Brackets) {
   // Offset of an array sub-field.  TODO: Should we allow vector elements?
   if (!CurrentType->isDependentType()) {
 const ArrayType *AT = Context.getAsArrayType(CurrentType);
@@ -16682,6 +16682,10 @@
   << SourceRange(Components[0].LocStart, OC.LocEnd)
   << CurrentType))
 DidWarnAboutNonPOD = true;
+
+  if (OC.Kind == OffsetOfComponent::Qualifier &&
+  RD->getIdentifier() == OC.U.IdentInfo)
+continue;
 }
 
 // Look for the field.
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2635,16 +2635,23 @@
 SmallVector Comps;
 
 Comps.push_back(Sema::OffsetOfComponent());
-Comps.back().isBrackets = false;
+Comps.back().Kind = Sema::OffsetOfComponent::Identifier;
 Comps.back().U.IdentInfo = Tok.getIdentifierInfo();
 Comps.back().LocStart = Comps.back().LocEnd = ConsumeToken();
 
 // FIXME: This loop leaks the index expressions on error.
 while (true) {
-  if (Tok.is(tok::period)) {
+  if (Tok.is(tok::period) || Tok.is(tok::coloncolon)) {
 // offsetof-member-designator: offsetof-member-designator '.' identifier
+if (Tok.is(tok::coloncolon) && getLangOpts().CPlusPlus) {
+  Comps.back().Kind = Sema::OffsetOfComponent::Qualifier;
+} else if (Tok.is(tok::coloncolon) && !getLangOpts().CPlusPlus) {
+  Res = ExprError();
+  break;
+}
+
 Comps.push_back(Sema::OffsetOfComponent());
-Comps.back().isBrackets = false;
+Comps.back().Kind = Sema::OffsetOfComponent::Identifier;
 Comps.back().LocStart = ConsumeToken();
 
 if (Tok.isNot(tok::identifier)) {
@@ -2660,7 +2667,7 @@
 
 // offsetof-member-designator: offsetof-member-design '[' expression ']'
 Comps.push_back(Sema::OffsetOfComponent());
-Comps.back().isBrackets = true;
+Comps.back().Kind = Sema::OffsetOfComponent::Brackets;
 BalancedDelimiterTracker ST(*this, tok::l_square);
 ST.consumeOpen();
 Comps.back().LocStart = ST.getOpenLocation();
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -6035,11 +6035,15 @@
   // 

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this!




Comment at: clang/docs/ReleaseNotes.rst:59
 
+- Improved `__builtin_offsetof` support, allowing qualified name in member 
designator.
 

You should also link to https://github.com/llvm/llvm-project/issues/64154 here 
to tie it back to a bug report.



Comment at: clang/include/clang/Sema/Sema.h:6038
 SourceLocation LocStart, LocEnd;
 bool isBrackets;  // true if [expr], false if .ident
+bool isQualifier;

I think we should combine `isBrackets` and `isQualifier` since a component 
can't be both at the same time anyway. e.g.,
```
enum {
  Brackets,   // U.E is valid
  Identifier, // U.IdentInfo is valid
  Qualifier,  // Nothing in U is valid
} Kind;
```



Comment at: clang/test/Sema/offsetof.c:77-79
+struct X2 { int a; };
+int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1];
+int x3[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // 
expected-error{{no member named 'X2'}}

Neither of these should be valid in C -- there is no way to name subobjects in 
C like there is in C++.



Comment at: clang/test/SemaCXX/offsetof.cpp:104
+struct X2 { int a; static int static_a; };
+int x2[__builtin_offsetof(struct X2, X2::static_a) == 0 ? 1 : -1]; // 
expected-error{{no member named 'static_a'}}

It'd be good to add the `int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : 
-1];` test here from the C test.


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Yichi Lee via Phabricator via cfe-commits]

yichi170 updated this revision to Diff 547764.
yichi170 marked an inline comment as not done.
yichi170 added a comment.

updated test in `test/SemaCXX/offsetof.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157201

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/offsetof.c
  clang/test/SemaCXX/offsetof.cpp


Index: clang/test/SemaCXX/offsetof.cpp
===
--- clang/test/SemaCXX/offsetof.cpp
+++ clang/test/SemaCXX/offsetof.cpp
@@ -98,3 +98,7 @@
 B x;
   }, a);
 }
+
+// https://github.com/llvm/llvm-project/issues/64154
+struct X2 { int a; static int static_a; };
+int x2[__builtin_offsetof(struct X2, X2::static_a) == 0 ? 1 : -1]; // 
expected-error{{no member named 'static_a'}}
Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -73,3 +73,8 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // 
expected-warning{{indirection of non-volatile null pointer}} 
expected-note{{__builtin_trap}}
 }
 
+// https://github.com/llvm/llvm-project/issues/64154
+struct X2 { int a; };
+int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1];
+int x3[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // 
expected-error{{no member named 'X2'}}
+
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16663,6 +16663,9 @@
<< CurrentType);
 RecordDecl *RD = RC->getDecl();
 
+if (OC.isQualifier && RD->getIdentifier() == OC.U.IdentInfo)
+  continue;
+
 // C++ [lib.support.types]p5:
 //   The macro offsetof accepts a restricted set of type arguments in this
 //   International Standard. type shall be a POD structure or a POD union
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2641,10 +2641,13 @@
 
 // FIXME: This loop leaks the index expressions on error.
 while (true) {
-  if (Tok.is(tok::period)) {
+  if (Tok.is(tok::period) || Tok.is(tok::coloncolon)) {
 // offsetof-member-designator: offsetof-member-designator '.' 
identifier
+if (Tok.is(tok::coloncolon))
+  Comps.back().isQualifier = true;
 Comps.push_back(Sema::OffsetOfComponent());
 Comps.back().isBrackets = false;
+Comps.back().isQualifier = false;
 Comps.back().LocStart = ConsumeToken();
 
 if (Tok.isNot(tok::identifier)) {
@@ -2661,6 +2664,7 @@
 // offsetof-member-designator: offsetof-member-design '[' expression 
']'
 Comps.push_back(Sema::OffsetOfComponent());
 Comps.back().isBrackets = true;
+Comps.back().isQualifier = false;
 BalancedDelimiterTracker ST(*this, tok::l_square);
 ST.consumeOpen();
 Comps.back().LocStart = ST.getOpenLocation();
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -6036,6 +6036,7 @@
   struct OffsetOfComponent {
 SourceLocation LocStart, LocEnd;
 bool isBrackets;  // true if [expr], false if .ident
+bool isQualifier;
 union {
   IdentifierInfo *IdentInfo;
   Expr *E;
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -56,6 +56,7 @@
 
 C++ Language Changes
 
+- Improved `__builtin_offsetof` support, allowing qualified name in member 
designator.
 
 C++20 Feature Support
 ^


Index: clang/test/SemaCXX/offsetof.cpp
===
--- clang/test/SemaCXX/offsetof.cpp
+++ clang/test/SemaCXX/offsetof.cpp
@@ -98,3 +98,7 @@
 B x;
   }, a);
 }
+
+// https://github.com/llvm/llvm-project/issues/64154
+struct X2 { int a; static int static_a; };
+int x2[__builtin_offsetof(struct X2, X2::static_a) == 0 ? 1 : -1]; // expected-error{{no member named 'static_a'}}
Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -73,3 +73,8 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // expected-warning{{indirection of non-volatile null pointer}} expected-note{{__builtin_trap}}
 }
 
+// https://github.com/llvm/llvm-project/issues/64154
+struct X2 { int a; };
+int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1];
+int x3[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // 

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/test/Sema/offsetof.c:79
+int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1];
+int x3[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // 
expected-error{{no member named 'X2'}}
+

yichi170 wrote:
> Fznamznon wrote:
> > It probably makes sense to also add a test with static member of a class, 
> > like Aaron mentioned in GitHub - 
> > https://github.com/llvm/llvm-project/issues/64154#issuecomment-1653468938 .
> Hi @Fznamznon, I found that clang and clang++ output different errors with 
> the following test.
> ```
> struct X2 { int a; static int static_a; };
> int x4[__builtin_offsetof(struct X2, X2::static_a) == 0 ? 1 : -1];
> // expected-error{{no member named 'static_a'}}
> ```
> C doesn't allow to have a static member in the struct, so is there any way to 
> write the correct comment to make the test pass?
the tests should be in `test/SemaCXX/offsetof.cpp`, where you can use C++


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Yichi Lee via Phabricator via cfe-commits]

yichi170 added inline comments.



Comment at: clang/test/Sema/offsetof.c:79
+int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1];
+int x3[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // 
expected-error{{no member named 'X2'}}
+

Fznamznon wrote:
> It probably makes sense to also add a test with static member of a class, 
> like Aaron mentioned in GitHub - 
> https://github.com/llvm/llvm-project/issues/64154#issuecomment-1653468938 .
Hi @Fznamznon, I found that clang and clang++ output different errors with the 
following test.
```
struct X2 { int a; static int static_a; };
int x4[__builtin_offsetof(struct X2, X2::static_a) == 0 ? 1 : -1];
// expected-error{{no member named 'static_a'}}
```
C doesn't allow to have a static member in the struct, so is there any way to 
write the correct comment to make the test pass?


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Yichi Lee via Phabricator via cfe-commits]

yichi170 updated this revision to Diff 547703.
yichi170 added a comment.

updated ReleaseNotes and applied suggestions. The related issue #63443 
 is still not fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157201

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/offsetof.c


Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -73,3 +73,8 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // 
expected-warning{{indirection of non-volatile null pointer}} 
expected-note{{__builtin_trap}}
 }
 
+// https://github.com/llvm/llvm-project/issues/64154
+struct X2 { int a; };
+int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1];
+int x3[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // 
expected-error{{no member named 'X2'}}
+
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16663,6 +16663,9 @@
<< CurrentType);
 RecordDecl *RD = RC->getDecl();
 
+if (OC.isQualifier && RD->getIdentifier() == OC.U.IdentInfo)
+  continue;
+
 // C++ [lib.support.types]p5:
 //   The macro offsetof accepts a restricted set of type arguments in this
 //   International Standard. type shall be a POD structure or a POD union
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2641,10 +2641,13 @@
 
 // FIXME: This loop leaks the index expressions on error.
 while (true) {
-  if (Tok.is(tok::period)) {
+  if (Tok.is(tok::period) || Tok.is(tok::coloncolon)) {
 // offsetof-member-designator: offsetof-member-designator '.' 
identifier
+if (Tok.is(tok::coloncolon))
+  Comps.back().isQualifier = true;
 Comps.push_back(Sema::OffsetOfComponent());
 Comps.back().isBrackets = false;
+Comps.back().isQualifier = false;
 Comps.back().LocStart = ConsumeToken();
 
 if (Tok.isNot(tok::identifier)) {
@@ -2661,6 +2664,7 @@
 // offsetof-member-designator: offsetof-member-design '[' expression 
']'
 Comps.push_back(Sema::OffsetOfComponent());
 Comps.back().isBrackets = true;
+Comps.back().isQualifier = false;
 BalancedDelimiterTracker ST(*this, tok::l_square);
 ST.consumeOpen();
 Comps.back().LocStart = ST.getOpenLocation();
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -6036,6 +6036,7 @@
   struct OffsetOfComponent {
 SourceLocation LocStart, LocEnd;
 bool isBrackets;  // true if [expr], false if .ident
+bool isQualifier;
 union {
   IdentifierInfo *IdentInfo;
   Expr *E;
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -56,6 +56,7 @@
 
 C++ Language Changes
 
+- Improved `__builtin_offsetof` support, allowing qualified name in member 
designator.
 
 C++20 Feature Support
 ^


Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -73,3 +73,8 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // expected-warning{{indirection of non-volatile null pointer}} expected-note{{__builtin_trap}}
 }
 
+// https://github.com/llvm/llvm-project/issues/64154
+struct X2 { int a; };
+int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1];
+int x3[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // expected-error{{no member named 'X2'}}
+
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16663,6 +16663,9 @@
<< CurrentType);
 RecordDecl *RD = RC->getDecl();
 
+if (OC.isQualifier && RD->getIdentifier() == OC.U.IdentInfo)
+  continue;
+
 // C++ [lib.support.types]p5:
 //   The macro offsetof accepts a restricted set of type arguments in this
 //   International Standard. type shall be a POD structure or a POD union
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2641,10 +2641,13 @@
 
 // FIXME: This loop leaks the index expressions on error.
 

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added inline comments.



Comment at: clang/test/Sema/offsetof.c:79
+int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1];
+int x3[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // 
expected-error{{no member named 'X2'}}
+

It probably makes sense to also add a test with static member of a class, like 
Aaron mentioned in GitHub - 
https://github.com/llvm/llvm-project/issues/64154#issuecomment-1653468938 .


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Yichi Lee via Phabricator via cfe-commits]

yichi170 added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:16696-16697
+
+  IdentifierInfo *II = RD->getIdentifier();
+  if (II == OC.U.IdentInfo && OC.isQualifier)
+continue;

tbaeder wrote:
> 
I'll fix it along with the ReleaseNotes.


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Yichi Lee via Phabricator via cfe-commits]

yichi170 updated this revision to Diff 547542.
yichi170 added a comment.

[Clang] Support qualified name as member designator in offsetof


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157201

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/offsetof.c


Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -73,3 +73,8 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // 
expected-warning{{indirection of non-volatile null pointer}} 
expected-note{{__builtin_trap}}
 }
 
+// https://github.com/llvm/llvm-project/issues/64154
+struct X2 { int a; };
+int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1];
+int x3[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // 
expected-error{{no member named 'X2'}}
+
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16692,6 +16692,10 @@
 if (!MemberDecl) {
   if ((IndirectMemberDecl = R.getAsSingle()))
 MemberDecl = IndirectMemberDecl->getAnonField();
+
+  IdentifierInfo *II = RD->getIdentifier();
+  if (II == OC.U.IdentInfo && OC.isQualifier)
+continue;
 }
 
 if (!MemberDecl) {
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2641,10 +2641,13 @@
 
 // FIXME: This loop leaks the index expressions on error.
 while (true) {
-  if (Tok.is(tok::period)) {
+  if (Tok.is(tok::period) || Tok.is(tok::coloncolon)) {
 // offsetof-member-designator: offsetof-member-designator '.' 
identifier
+if (Tok.is(tok::coloncolon))
+  Comps.back().isQualifier = true;
 Comps.push_back(Sema::OffsetOfComponent());
 Comps.back().isBrackets = false;
+Comps.back().isQualifier = false;
 Comps.back().LocStart = ConsumeToken();
 
 if (Tok.isNot(tok::identifier)) {
@@ -2661,6 +2664,7 @@
 // offsetof-member-designator: offsetof-member-design '[' expression 
']'
 Comps.push_back(Sema::OffsetOfComponent());
 Comps.back().isBrackets = true;
+Comps.back().isQualifier = false;
 BalancedDelimiterTracker ST(*this, tok::l_square);
 ST.consumeOpen();
 Comps.back().LocStart = ST.getOpenLocation();
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -6036,6 +6036,7 @@
   struct OffsetOfComponent {
 SourceLocation LocStart, LocEnd;
 bool isBrackets;  // true if [expr], false if .ident
+bool isQualifier;
 union {
   IdentifierInfo *IdentInfo;
   Expr *E;


Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -73,3 +73,8 @@
   return __builtin_offsetof(Array, array[*(int*)0]); // expected-warning{{indirection of non-volatile null pointer}} expected-note{{__builtin_trap}}
 }
 
+// https://github.com/llvm/llvm-project/issues/64154
+struct X2 { int a; };
+int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1];
+int x3[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // expected-error{{no member named 'X2'}}
+
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16692,6 +16692,10 @@
 if (!MemberDecl) {
   if ((IndirectMemberDecl = R.getAsSingle()))
 MemberDecl = IndirectMemberDecl->getAnonField();
+
+  IdentifierInfo *II = RD->getIdentifier();
+  if (II == OC.U.IdentInfo && OC.isQualifier)
+continue;
 }
 
 if (!MemberDecl) {
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2641,10 +2641,13 @@
 
 // FIXME: This loop leaks the index expressions on error.
 while (true) {
-  if (Tok.is(tok::period)) {
+  if (Tok.is(tok::period) || Tok.is(tok::coloncolon)) {
 // offsetof-member-designator: offsetof-member-designator '.' identifier
+if (Tok.is(tok::coloncolon))
+  Comps.back().isQualifier = true;
 Comps.push_back(Sema::OffsetOfComponent());
 Comps.back().isBrackets = false;
+Comps.back().isQualifier = false;
 Comps.back().LocStart = ConsumeToken();
 
 if (Tok.isNot(tok::identifier)) {
@@ -2661,6 +2664,7 @@
 // offsetof-member-designator: offsetof-member-design '[' expression ']'
 

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

Thanks a lot for working on this. Can you add tests (to 
`clang/test/SemaCXX/offsetof.cpp`)?
The change will also need an entry in `clang/docs/ReleaseNotes.rst` , in the 
"C++ Language Changes"  section


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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

You're missing tests.




Comment at: clang/lib/Sema/SemaExpr.cpp:16696-16697
+
+  IdentifierInfo *II = RD->getIdentifier();
+  if (II == OC.U.IdentInfo && OC.isQualifier)
+continue;




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

[PATCH] D157201: [Clang] Support qualified name as member designator in offsetof

[Yichi Lee via Phabricator via cfe-commits]

yichi170 created this revision.
Herald added a project: All.
yichi170 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157201

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaExpr.cpp


Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16692,6 +16692,10 @@
 if (!MemberDecl) {
   if ((IndirectMemberDecl = R.getAsSingle()))
 MemberDecl = IndirectMemberDecl->getAnonField();
+
+  IdentifierInfo *II = RD->getIdentifier();
+  if (II == OC.U.IdentInfo && OC.isQualifier)
+continue;
 }
 
 if (!MemberDecl) {
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2641,10 +2641,13 @@
 
 // FIXME: This loop leaks the index expressions on error.
 while (true) {
-  if (Tok.is(tok::period)) {
+  if (Tok.is(tok::period) || Tok.is(tok::coloncolon)) {
 // offsetof-member-designator: offsetof-member-designator '.' 
identifier
+if (Tok.is(tok::coloncolon))
+  Comps.back().isQualifier = true;
 Comps.push_back(Sema::OffsetOfComponent());
 Comps.back().isBrackets = false;
+Comps.back().isQualifier = false;
 Comps.back().LocStart = ConsumeToken();
 
 if (Tok.isNot(tok::identifier)) {
@@ -2661,6 +2664,7 @@
 // offsetof-member-designator: offsetof-member-design '[' expression 
']'
 Comps.push_back(Sema::OffsetOfComponent());
 Comps.back().isBrackets = true;
+Comps.back().isQualifier = false;
 BalancedDelimiterTracker ST(*this, tok::l_square);
 ST.consumeOpen();
 Comps.back().LocStart = ST.getOpenLocation();
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -6036,6 +6036,7 @@
   struct OffsetOfComponent {
 SourceLocation LocStart, LocEnd;
 bool isBrackets;  // true if [expr], false if .ident
+bool isQualifier;
 union {
   IdentifierInfo *IdentInfo;
   Expr *E;


Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -16692,6 +16692,10 @@
 if (!MemberDecl) {
   if ((IndirectMemberDecl = R.getAsSingle()))
 MemberDecl = IndirectMemberDecl->getAnonField();
+
+  IdentifierInfo *II = RD->getIdentifier();
+  if (II == OC.U.IdentInfo && OC.isQualifier)
+continue;
 }
 
 if (!MemberDecl) {
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2641,10 +2641,13 @@
 
 // FIXME: This loop leaks the index expressions on error.
 while (true) {
-  if (Tok.is(tok::period)) {
+  if (Tok.is(tok::period) || Tok.is(tok::coloncolon)) {
 // offsetof-member-designator: offsetof-member-designator '.' identifier
+if (Tok.is(tok::coloncolon))
+  Comps.back().isQualifier = true;
 Comps.push_back(Sema::OffsetOfComponent());
 Comps.back().isBrackets = false;
+Comps.back().isQualifier = false;
 Comps.back().LocStart = ConsumeToken();
 
 if (Tok.isNot(tok::identifier)) {
@@ -2661,6 +2664,7 @@
 // offsetof-member-designator: offsetof-member-design '[' expression ']'
 Comps.push_back(Sema::OffsetOfComponent());
 Comps.back().isBrackets = true;
+Comps.back().isQualifier = false;
 BalancedDelimiterTracker ST(*this, tok::l_square);
 ST.consumeOpen();
 Comps.back().LocStart = ST.getOpenLocation();
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -6036,6 +6036,7 @@
   struct OffsetOfComponent {
 SourceLocation LocStart, LocEnd;
 bool isBrackets;  // true if [expr], false if .ident
+bool isQualifier;
 union {
   IdentifierInfo *IdentInfo;
   Expr *E;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits