[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG456d072405d2: Reland: [clang][AST] Print name instead of 
type when diagnosing uninitialized… (authored by hazohelet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146358

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/Interp.cpp
  clang/test/AST/Interp/cxx20.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -761,7 +761,7 @@
 };
 
 S s1; // expected-error {{call to consteval function 'NamespaceScopeConsteval::S::S' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 template 
 struct T {
@@ -770,7 +770,7 @@
 };
 
 T t; // expected-error {{call to consteval function 'NamespaceScopeConsteval::T::T' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 } // namespace NamespaceScopeConsteval
 
Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -409,12 +409,12 @@
 b.a.x = 2;
 return b;
   }
-  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
   static_assert(return_uninit().a.x == 2);
   constexpr A return_uninit_struct() {
 B b = {.b = 1};
 b.a.x = 2;
-return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject of type 'int' is not initialized}}
+return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject 'y' is not initialized}}
   }
   // Note that this is rejected even though return_uninit() is accepted, and
   // return_uninit() copies the same stuff wrapped in a union.
@@ -558,7 +558,7 @@
 }
   };
   constinit X x1(true);
-  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject of type 'int' is not initialized}}
+  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject 'n' is not initialized}}
 
   struct Y {
 struct Z { int n; }; // expected-note {{here}}
@@ -577,7 +577,7 @@
   };
   // FIXME: This is working around clang not implementing DR2026. With that
   // fixed, we should be able to test this without the injected copy.
-  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject 'n' is not initialized}}
   constexpr Y y1 = copy(Y());
   static_assert(y1.z1.n == 1 && y1.z2.n == 2 && y1.z3.n == 3);
 
Index: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
+++ clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
@@ -360,7 +360,7 @@
   // The constructor is still 'constexpr' here, but the result is not intended
   // to be a constant expression. The standard is not clear on how this should
   // work.
-  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
 
   constexpr int k = V().x; // FIXME: ok?
 }
Index: clang/test/AST/Interp/cxx20.cpp
===
--- clang/test/AST/Interp/cxx20.cpp
+++ clang/test/AST/Interp/cxx20.cpp
@@ -143,9 +143,9 @@
 constexpr A() {}
   };
   constexpr A a; // expected-error {{must be initialized by a constant expression}} \
- // expected-note {{subobject of type 'int' is not initialized}} \
+ // expected-note {{subobject 'a' is not initialized}} \
  // ref-error {{must be initialized by a constant expression}} \
- // ref-note {{subobject of type 'int' is not initialized}}
+ // ref-note {{subobject 

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

In D146358#4364058 , @hazohelet wrote:

> The cause of the regression has been fixed in the other patch.
> This is a resubmission with rebase to the upstream.

LGTM, feel free to land again. I believe the failing precommit CI test is 
unrelated to the changes here, but please keep an eye on the post-commit bots 
just in case.


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet updated this revision to Diff 524655.
hazohelet added a comment.

The cause of the regression has been fixed in the other patch.
This is a resubmission with rebase to the upstream.


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

https://reviews.llvm.org/D146358

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/Interp.cpp
  clang/test/AST/Interp/cxx20.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -761,7 +761,7 @@
 };
 
 S s1; // expected-error {{call to consteval function 'NamespaceScopeConsteval::S::S' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 template 
 struct T {
@@ -770,7 +770,7 @@
 };
 
 T t; // expected-error {{call to consteval function 'NamespaceScopeConsteval::T::T' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 } // namespace NamespaceScopeConsteval
 
Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -409,12 +409,12 @@
 b.a.x = 2;
 return b;
   }
-  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
   static_assert(return_uninit().a.x == 2);
   constexpr A return_uninit_struct() {
 B b = {.b = 1};
 b.a.x = 2;
-return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject of type 'int' is not initialized}}
+return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject 'y' is not initialized}}
   }
   // Note that this is rejected even though return_uninit() is accepted, and
   // return_uninit() copies the same stuff wrapped in a union.
@@ -558,7 +558,7 @@
 }
   };
   constinit X x1(true);
-  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject of type 'int' is not initialized}}
+  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject 'n' is not initialized}}
 
   struct Y {
 struct Z { int n; }; // expected-note {{here}}
@@ -577,7 +577,7 @@
   };
   // FIXME: This is working around clang not implementing DR2026. With that
   // fixed, we should be able to test this without the injected copy.
-  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject 'n' is not initialized}}
   constexpr Y y1 = copy(Y());
   static_assert(y1.z1.n == 1 && y1.z2.n == 2 && y1.z3.n == 3);
 
Index: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
+++ clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
@@ -360,7 +360,7 @@
   // The constructor is still 'constexpr' here, but the result is not intended
   // to be a constant expression. The standard is not clear on how this should
   // work.
-  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
 
   constexpr int k = V().x; // FIXME: ok?
 }
Index: clang/test/AST/Interp/cxx20.cpp
===
--- clang/test/AST/Interp/cxx20.cpp
+++ clang/test/AST/Interp/cxx20.cpp
@@ -143,9 +143,9 @@
 constexpr A() {}
   };
   constexpr A a; // expected-error {{must be initialized by a constant expression}} \
- // expected-note {{subobject of type 'int' is not initialized}} \
+ // expected-note {{subobject 'a' is not initialized}} \
  // ref-error {{must be initialized by a constant expression}} \
- // ref-note {{subobject of type 'int' is not initialized}}
+ // ref-note {{subobject 'a' is not initialized}}
 
 
   class Base {
@@ -161,18 +161,18 @@
 constexpr Derived() : Base() {}   };
 
   

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

In D146358#4357276 , @erichkeane 
wrote:

> In D146358#4357106 , @hazohelet 
> wrote:
>
>> Thanks for the revert!
>>
>> It seems that the problem is around the list initializer in template 
>> variable definition.
>> Minimum reproducible example:
>> `template  constexpr T foo{};`
>>
>> When the `TextNodeDumper` enabled through `-ast-dump` visits a constexpr 
>> `VarDecl` with initializer, it evaluates the `VarDecl` and prints the value 
>> if evaluation comes successful.
>> (https://github.com/llvm/llvm-project/blob/55903151a2a505284ce3fcd955b1d394df0b55ea/clang/lib/AST/TextNodeDumper.cpp#L1825)
>> The `VarDecl::evaluateValue()` does not show the notes generated during its 
>> failed evaluation. The uninitialized-subobject note is actually generated 
>> BEFORE this patch when compiling the reproducers with `-ast-dump` flag on, 
>> but is not shown to the user.
>> The message should not be generated here and I assume this is an internal 
>> bug that we have to fix before resubmitting this patch.
>
> It seems to me that perhaps the assert you added is not appropriate?  
> Instead, could we have the OLD diagnostic 'back' as an alternate form (when 
> this is empty)?

If we use the old diagnostic, `subobject of type 'const T' is not initialized` 
will be generated internally when clang dumps AST of the reproducer 
`template constexpr T foo{};`. This is not what we want, right? I 
want to keep the assert here because it seems to be a nice bug catcher so far.

I did some more investigation and now I am sure that `TextNodeDumper` should 
not evaluate the initializer of constexpr-qualified `VarDecl` if it is an 
variable template definition(in the code I provided as permalink to GitHub). 
This approach fixes the assertion failure in the regression.
I opened another differential D151033  for 
that change, and I think it would be safe to reland this patch once the other 
is landed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D146358#4357106 , @hazohelet wrote:

> In D146358#4352792 , @erichkeane 
> wrote:
>
>> In D146358#4351787 , @hazohelet 
>> wrote:
>>
>>> In D146358#4350810 , @erichkeane 
>>> wrote:
>>>
 Our downstream discovered that this causes a regression when compiling 
 `utility` with `ast-dump`.  I've put up a minimal reproducer: 
 https://godbolt.org/z/oaezas7Ws

 @hazohelet : Will you be able to look into this soon?
>>>
>>> Thanks for the report and the reproducer.
>>> I cannot take sufficient time for about 36 hours, but I'll be able to 
>>> investigate it after that.
>>
>> That is long enough away that I unfortunately have to revert (see 
>> 34e49d3e85a6dd03856af0fb4b7f8d8ae1f4f06a 
>> ). 
>> Please note that in the LLVM project reverts are common/frequent (we have a 
>> policy of 'revert often'), and should not be taken negatively!  Please feel 
>> free to re-submit this patch with a fix/the test provided, and we'll 
>> re-review promptly!
>
> Thanks for the revert!
>
> It seems that the problem is around the list initializer in template variable 
> definition.
> Minimum reproducible example:
> `template  constexpr T foo{};`
>
> When the `TextNodeDumper` enabled through `-ast-dump` visits a constexpr 
> `VarDecl` with initializer, it evaluates the `VarDecl` and prints the value 
> if evaluation comes successful.
> (https://github.com/llvm/llvm-project/blob/55903151a2a505284ce3fcd955b1d394df0b55ea/clang/lib/AST/TextNodeDumper.cpp#L1825)
> The `VarDecl::evaluateValue()` does not show the notes generated during its 
> failed evaluation. The uninitialized-subobject note is actually generated 
> BEFORE this patch when compiling the reproducers with `-ast-dump` flag on, 
> but is not shown to the user.
> The message should not be generated here and I assume this is an internal bug 
> that we have to fix before resubmitting this patch.

It seems to me that perhaps the assert you added is not appropriate?  Instead, 
could we have the OLD diagnostic 'back' as an alternate form (when this is 
empty)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

In D146358#4352792 , @erichkeane 
wrote:

> In D146358#4351787 , @hazohelet 
> wrote:
>
>> In D146358#4350810 , @erichkeane 
>> wrote:
>>
>>> Our downstream discovered that this causes a regression when compiling 
>>> `utility` with `ast-dump`.  I've put up a minimal reproducer: 
>>> https://godbolt.org/z/oaezas7Ws
>>>
>>> @hazohelet : Will you be able to look into this soon?
>>
>> Thanks for the report and the reproducer.
>> I cannot take sufficient time for about 36 hours, but I'll be able to 
>> investigate it after that.
>
> That is long enough away that I unfortunately have to revert (see 
> 34e49d3e85a6dd03856af0fb4b7f8d8ae1f4f06a 
> ). 
> Please note that in the LLVM project reverts are common/frequent (we have a 
> policy of 'revert often'), and should not be taken negatively!  Please feel 
> free to re-submit this patch with a fix/the test provided, and we'll 
> re-review promptly!

Thanks for the revert!

It seems that the problem is around the list initializer in template variable 
definition.
Minimum reproducible example:
`template  constexpr T foo{};`

When the `TextNodeDumper` enabled through `-ast-dump` visits a constexpr 
`VarDecl` with initializer, it evaluates the `VarDecl` and prints the value if 
evaluation comes successful.
(https://github.com/llvm/llvm-project/blob/55903151a2a505284ce3fcd955b1d394df0b55ea/clang/lib/AST/TextNodeDumper.cpp#L1825)
The `VarDecl::evaluateValue()` does not show the notes generated during its 
failed evaluation. The uninitialized-subobject note is actually generated 
BEFORE this patch when compiling the reproducers with `-ast-dump` flag on, but 
is not shown to the user.
The message should not be generated here and I assume this is an internal bug 
that we have to fix before resubmitting this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D146358#4351787 , @hazohelet wrote:

> In D146358#4350810 , @erichkeane 
> wrote:
>
>> Our downstream discovered that this causes a regression when compiling 
>> `utility` with `ast-dump`.  I've put up a minimal reproducer: 
>> https://godbolt.org/z/oaezas7Ws
>>
>> @hazohelet : Will you be able to look into this soon?
>
> Thanks for the report and the reproducer.
> I cannot take sufficient time for about 36 hours, but I'll be able to 
> investigate it after that.

That is long enough away that I unfortunately have to revert (see 
34e49d3e85a6dd03856af0fb4b7f8d8ae1f4f06a 
). Please 
note that in the LLVM project reverts are common/frequent (we have a policy of 
'revert often'), and should not be taken negatively!  Please feel free to 
re-submit this patch with a fix/the test provided, and we'll re-review promptly!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

In D146358#4350810 , @erichkeane 
wrote:

> Our downstream discovered that this causes a regression when compiling 
> `utility` with `ast-dump`.  I've put up a minimal reproducer: 
> https://godbolt.org/z/oaezas7Ws
>
> @hazohelet : Will you be able to look into this soon?

Thanks for the report and the reproducer.
I cannot take sufficient time for about 36 hours, but I'll be able to 
investigate it after that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Slightly smaller reproducer: https://godbolt.org/z/W5zjrKaz9


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Our downstream discovered that this causes a regression when compiling 
`utility` with `ast-dump`.  I've put up a minimal reproducer: 
https://godbolt.org/z/oaezas7Ws

@hazohelet : Will you be able to look into this soon?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0e167fc0a214: [clang][AST] Print name instead of type when 
diagnosing uninitialized subobject… (authored by hazohelet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146358

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/Interp.cpp
  clang/test/AST/Interp/cxx20.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -761,7 +761,7 @@
 };
 
 S s1; // expected-error {{call to consteval function 'NamespaceScopeConsteval::S::S' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 template 
 struct T {
@@ -770,7 +770,7 @@
 };
 
 T t; // expected-error {{call to consteval function 'NamespaceScopeConsteval::T::T' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 } // namespace NamespaceScopeConsteval
 
Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -409,12 +409,12 @@
 b.a.x = 2;
 return b;
   }
-  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
   static_assert(return_uninit().a.x == 2);
   constexpr A return_uninit_struct() {
 B b = {.b = 1};
 b.a.x = 2;
-return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject of type 'int' is not initialized}}
+return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject 'y' is not initialized}}
   }
   // Note that this is rejected even though return_uninit() is accepted, and
   // return_uninit() copies the same stuff wrapped in a union.
@@ -558,7 +558,7 @@
 }
   };
   constinit X x1(true);
-  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject of type 'int' is not initialized}}
+  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject 'n' is not initialized}}
 
   struct Y {
 struct Z { int n; }; // expected-note {{here}}
@@ -577,7 +577,7 @@
   };
   // FIXME: This is working around clang not implementing DR2026. With that
   // fixed, we should be able to test this without the injected copy.
-  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject 'n' is not initialized}}
   constexpr Y y1 = copy(Y());
   static_assert(y1.z1.n == 1 && y1.z2.n == 2 && y1.z3.n == 3);
 
Index: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
+++ clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
@@ -360,7 +360,7 @@
   // The constructor is still 'constexpr' here, but the result is not intended
   // to be a constant expression. The standard is not clear on how this should
   // work.
-  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
 
   constexpr int k = V().x; // FIXME: ok?
 }
Index: clang/test/AST/Interp/cxx20.cpp
===
--- clang/test/AST/Interp/cxx20.cpp
+++ clang/test/AST/Interp/cxx20.cpp
@@ -143,9 +143,9 @@
 constexpr A() {}
   };
   constexpr A a; // expected-error {{must be initialized by a constant expression}} \
- // expected-note {{subobject of type 'int' is not initialized}} \
+ // expected-note {{subobject 'a' is not initialized}} \
  // ref-error {{must be initialized by a constant expression}} \
- // ref-note {{subobject of type 'int' is not initialized}}
+ // ref-note 

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

In D146358#4343281 , @aaron.ballman 
wrote:

> LGTM! Did you end up requesting commit access? If not, now is probably a good 
> time: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Thanks, I have commit access now, so I'll land this patch myself


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM! Did you end up requesting commit access? If not, now is probably a good 
time: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access




Comment at: clang/lib/AST/ExprConstant.cpp:2357-2361
+if (SubobjectDecl) {
+  Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << 
SubobjectDecl;
+  Info.Note(SubobjectDecl->getLocation(),
+diag::note_constexpr_subobject_declared_here);
+}

hazohelet wrote:
> tbaeder wrote:
> > aaron.ballman wrote:
> > > hazohelet wrote:
> > > > aaron.ballman wrote:
> > > > > Hmm, this breaks one of the contracts of our constexpr evaluation 
> > > > > engine, doesn't it? My understanding is that if constexpr evaluation 
> > > > > fails, we will have emitted a note diagnostic for why it failed. But 
> > > > > if the caller doesn't pass a nonnull `SubobjectDecl`, we'll return 
> > > > > `false` but we won't issue a diagnostic.
> > > > > 
> > > > > I'm surprised no tests lost notes as a result of this change, that 
> > > > > suggests we're missing test coverage for the cases where nullptr is 
> > > > > passed in explicitly to this function.
> > > > Yeah, I was looking into when `SubobjectDecl` can be null here. I 
> > > > `assert`ed the non-nullness of `SubobjectDecl` before and found that 
> > > > there exists two lines of code 
> > > > (https://github.com/llvm/llvm-project/blob/22a3f974d35da89247c0396594f2e4cd592742eb/clang/test/SemaCXX/attr-weak.cpp#L49
> > > >  and 
> > > > https://github.com/llvm/llvm-project/blob/abf4a8cb15d4faf04ee0da14e37d7349d3bde9a1/clang/test/CodeGenCXX/weak-external.cpp#L97)
> > > >  in the test codes that nulls here.
> > > > It seems they are doing the same thing, doing comparison against a 
> > > > pointer to a `[[gnu::weak]]` member. A simple reproducing code is here: 
> > > > https://godbolt.org/z/qn997n85n
> > > > As you can see from the compiler explorer, there's no note emitted here 
> > > > before the patch.
> > > > I inserted some `printf` into the code before this patch  and confirmed 
> > > > `Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << true << 
> > > > Type` was actually called when compiling the reproducing code and that 
> > > > somehow it is ignored. FWIW, `SubobjectLoc.isValid()` was `false` here.
> > > > It seems they are doing the same thing, doing comparison against a 
> > > > pointer to a [[gnu::weak]] member. A simple reproducing code is here: 
> > > > https://godbolt.org/z/qn997n85n
> > > > As you can see from the compiler explorer, there's no note emitted here 
> > > > before the patch.
> > > 
> > > I see a note generated there:
> > > ```
> > > :4:41: note: comparison against pointer to weak member 
> > > 'Weak::weak_method' can only be performed at runtime
> > > constexpr bool val = ::weak_method != nullptr;
> > > ^
> > > ```
> > > The situations I'm concerned about are the changes to 
> > > ExprConstant.cpp:2270 or line 2399 and so on.
> > The `FFDiag` call can just stay as it was, right? And then only the 
> > `Info.Note` call needs to be conditional for whether we have  a 
> > `SubobjectDecl`.
> In my understanding, the concern is that there could be note loss when 
> `Value.hasValue()` evaluates to `false` and at the same time `SubobjectDecl` 
> is `nullptr` explicitly passed in this change. 
> `[[gnu::weak]]` member pointer comparison is the only example of this 
> happening in the clang test codes where `Subobject` is `nullptr` passed at 
> ExprConstant.cpp:2454 in `CheckFullyInitialized`. In this case the 
> "uninitialized subobject" note was not displayed before this patch as well 
> because there is another note issued elsewhere. Thus, the note loss does not 
> happen.
> 
> BTW, I checked where the `[[gnu::weak]]` note is issued and I think there 
> might be a mistake in the return statements. The `return true` should be 
> `return false` because the constexpr evaluator fails to evaluate the 
> expression, right? Correct me if I am wrong. Here's the reference:
> https://github.com/llvm/llvm-project/blob/e58a49300e757ff61142f6abd227bd1437c1cf87/clang/lib/AST/ExprConstant.cpp#L13140-L13151
> 
> If we change them to `false`, `CheckFullyInitialized` will not be called in 
> this scenario, and we will  have no test cases where `SubobjectDecl` is 
> `nullptr` and at the same time `Value.hasValue()` is `false`. I am unsure 
> whether this is due to the lack of test coverage or it is guaranteed that 
> this condition does not hold here.
> 
> > The `FFDiag` call can just stay as it was, right? And then only the 
> > `Info.Note` call needs to be conditional for whether we have  a 
> > `SubobjectDecl`.
> If `SubobjectDecl` is null, it would cause segfault when the `FFDiag` note is 
> displayed, I think.
> 
> The best way I can think of would be to `assert(SubobjectDecl)` here, 

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet updated this revision to Diff 522165.
hazohelet added a comment.

Rebase and Ping


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

https://reviews.llvm.org/D146358

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/Interp.cpp
  clang/test/AST/Interp/cxx20.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -761,7 +761,7 @@
 };
 
 S s1; // expected-error {{call to consteval function 'NamespaceScopeConsteval::S::S' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 template 
 struct T {
@@ -770,7 +770,7 @@
 };
 
 T t; // expected-error {{call to consteval function 'NamespaceScopeConsteval::T::T' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 } // namespace NamespaceScopeConsteval
 
Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -409,12 +409,12 @@
 b.a.x = 2;
 return b;
   }
-  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
   static_assert(return_uninit().a.x == 2);
   constexpr A return_uninit_struct() {
 B b = {.b = 1};
 b.a.x = 2;
-return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject of type 'int' is not initialized}}
+return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject 'y' is not initialized}}
   }
   // Note that this is rejected even though return_uninit() is accepted, and
   // return_uninit() copies the same stuff wrapped in a union.
@@ -558,7 +558,7 @@
 }
   };
   constinit X x1(true);
-  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject of type 'int' is not initialized}}
+  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject 'n' is not initialized}}
 
   struct Y {
 struct Z { int n; }; // expected-note {{here}}
@@ -577,7 +577,7 @@
   };
   // FIXME: This is working around clang not implementing DR2026. With that
   // fixed, we should be able to test this without the injected copy.
-  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject 'n' is not initialized}}
   constexpr Y y1 = copy(Y());
   static_assert(y1.z1.n == 1 && y1.z2.n == 2 && y1.z3.n == 3);
 
Index: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
+++ clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
@@ -360,7 +360,7 @@
   // The constructor is still 'constexpr' here, but the result is not intended
   // to be a constant expression. The standard is not clear on how this should
   // work.
-  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
 
   constexpr int k = V().x; // FIXME: ok?
 }
Index: clang/test/AST/Interp/cxx20.cpp
===
--- clang/test/AST/Interp/cxx20.cpp
+++ clang/test/AST/Interp/cxx20.cpp
@@ -143,9 +143,9 @@
 constexpr A() {}
   };
   constexpr A a; // expected-error {{must be initialized by a constant expression}} \
- // expected-note {{subobject of type 'int' is not initialized}} \
+ // expected-note {{subobject 'a' is not initialized}} \
  // ref-error {{must be initialized by a constant expression}} \
- // ref-note {{subobject of type 'int' is not initialized}}
+ // ref-note {{subobject 'a' is not initialized}}
 
 
   class Base {
@@ -161,18 +161,18 @@
 constexpr Derived() : Base() {}   };
 
   constexpr Derived D; // expected-error {{must be initialized by a constant expression}} \
-  

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

Ping


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

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

tbaeder added a comment.

@aaron.ballman Are you OK with the approach taken here?


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet updated this revision to Diff 514252.
hazohelet added a comment.

Remove `gnu::weak` diff


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

https://reviews.llvm.org/D146358

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/Interp.cpp
  clang/test/AST/Interp/cxx20.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -761,7 +761,7 @@
 };
 
 S s1; // expected-error {{call to consteval function 'NamespaceScopeConsteval::S::S' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 template 
 struct T {
@@ -770,7 +770,7 @@
 };
 
 T t; // expected-error {{call to consteval function 'NamespaceScopeConsteval::T::T' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 } // namespace NamespaceScopeConsteval
 
Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -409,12 +409,12 @@
 b.a.x = 2;
 return b;
   }
-  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
   static_assert(return_uninit().a.x == 2);
   constexpr A return_uninit_struct() {
 B b = {.b = 1};
 b.a.x = 2;
-return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject of type 'int' is not initialized}}
+return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject 'y' is not initialized}}
   }
   // Note that this is rejected even though return_uninit() is accepted, and
   // return_uninit() copies the same stuff wrapped in a union.
@@ -558,7 +558,7 @@
 }
   };
   constinit X x1(true);
-  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject of type 'int' is not initialized}}
+  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject 'n' is not initialized}}
 
   struct Y {
 struct Z { int n; }; // expected-note {{here}}
@@ -577,7 +577,7 @@
   };
   // FIXME: This is working around clang not implementing DR2026. With that
   // fixed, we should be able to test this without the injected copy.
-  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject 'n' is not initialized}}
   constexpr Y y1 = copy(Y());
   static_assert(y1.z1.n == 1 && y1.z2.n == 2 && y1.z3.n == 3);
 
Index: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
+++ clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
@@ -359,7 +359,7 @@
   // The constructor is still 'constexpr' here, but the result is not intended
   // to be a constant expression. The standard is not clear on how this should
   // work.
-  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
 
   constexpr int k = V().x; // FIXME: ok?
 }
Index: clang/test/AST/Interp/cxx20.cpp
===
--- clang/test/AST/Interp/cxx20.cpp
+++ clang/test/AST/Interp/cxx20.cpp
@@ -143,9 +143,9 @@
 constexpr A() {}
   };
   constexpr A a; // expected-error {{must be initialized by a constant expression}} \
- // expected-note {{subobject of type 'int' is not initialized}} \
+ // expected-note {{subobject 'a' is not initialized}} \
  // ref-error {{must be initialized by a constant expression}} \
- // ref-note {{subobject of type 'int' is not initialized}}
+ // ref-note {{subobject 'a' is not initialized}}
 
 
   class Base {
@@ -161,18 +161,18 @@
 constexpr Derived() : Base() {}   };
 
   constexpr Derived D; // expected-error {{must be initialized by a constant expression}} \
-  

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

In D146358#4268327 , @tbaeder wrote:

> So, if I understand the code correctly, we call `CheckEvaluationResult` with 
> `SubObjectDecl=nullptr` when we're not checking an actual field but just an 
> array/record, so we can't run into this problem anyway, so the assert seems 
> fine.
>
> I don't fully understand the problem with the `gnu::weak` stuff, in any case, 
> adding it to  this patch seems wrong. https://godbolt.org/z/qn997n85n 
> //does// emit a note, but it's not the one this patch is about, so is that 
> even relevant?

Thanks. I opened another differential for the `gnu::weak` stuff (D148419 
).
If my understanding is correct, `Info.FFDiag` displays only the note of its 
first call and discard the notes after it. So, the uninitialized-note is not 
displayed.
You can confirm this by deleting only the FFDiag call 
(https://github.com/llvm/llvm-project/blob/6f7e5c0f1ac6cc3349a2e1479ac4208465b272c6/clang/lib/AST/ExprConstant.cpp#L13143-L13144)
 from the clang trunk and compile my godbolt code to see the note (`note: 
subobject of type 'const bool' is not initialized`)
Although the `return true` says the `[[gnu::weak]]` member pointer comparison 
is valid, constexpr engine considers the constant initialization has failed if 
there is note emitted during the initializer 
evaluation(https://github.com/llvm/llvm-project/blob/6f7e5c0f1ac6cc3349a2e1479ac4208465b272c6/clang/lib/AST/Decl.cpp#L2591-L2592)
 and it's why `[[gnu::weak]]` comparison note is actually emitted in spite of 
the wrong signal about the validness of the expression.


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

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

tbaeder added a comment.

So, if I understand the code correctly, we call `CheckEvaluationResult` with 
`SubObjectDecl=nullptr` when we're not checking an actual field but just an 
array/record, so we can't run into this problem anyway, so the assert seems 
fine.

I don't fully understand the problem with the `gnu::weak` stuff, in any case, 
adding it to  this patch seems wrong. https://godbolt.org/z/qn997n85n //does// 
emit a note, but it's not the one this patch is about, so is that even relevant?


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet updated this revision to Diff 511659.

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

https://reviews.llvm.org/D146358

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/Interp.cpp
  clang/test/AST/Interp/cxx20.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -761,7 +761,7 @@
 };
 
 S s1; // expected-error {{call to consteval function 'NamespaceScopeConsteval::S::S' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 template 
 struct T {
@@ -770,7 +770,7 @@
 };
 
 T t; // expected-error {{call to consteval function 'NamespaceScopeConsteval::T::T' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 } // namespace NamespaceScopeConsteval
 
Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -409,12 +409,12 @@
 b.a.x = 2;
 return b;
   }
-  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
   static_assert(return_uninit().a.x == 2);
   constexpr A return_uninit_struct() {
 B b = {.b = 1};
 b.a.x = 2;
-return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject of type 'int' is not initialized}}
+return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject 'y' is not initialized}}
   }
   // Note that this is rejected even though return_uninit() is accepted, and
   // return_uninit() copies the same stuff wrapped in a union.
@@ -558,7 +558,7 @@
 }
   };
   constinit X x1(true);
-  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject of type 'int' is not initialized}}
+  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject 'n' is not initialized}}
 
   struct Y {
 struct Z { int n; }; // expected-note {{here}}
@@ -577,7 +577,7 @@
   };
   // FIXME: This is working around clang not implementing DR2026. With that
   // fixed, we should be able to test this without the injected copy.
-  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject 'n' is not initialized}}
   constexpr Y y1 = copy(Y());
   static_assert(y1.z1.n == 1 && y1.z2.n == 2 && y1.z3.n == 3);
 
Index: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
+++ clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
@@ -359,7 +359,7 @@
   // The constructor is still 'constexpr' here, but the result is not intended
   // to be a constant expression. The standard is not clear on how this should
   // work.
-  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
 
   constexpr int k = V().x; // FIXME: ok?
 }
Index: clang/test/AST/Interp/cxx20.cpp
===
--- clang/test/AST/Interp/cxx20.cpp
+++ clang/test/AST/Interp/cxx20.cpp
@@ -143,9 +143,9 @@
 constexpr A() {}
   };
   constexpr A a; // expected-error {{must be initialized by a constant expression}} \
- // expected-note {{subobject of type 'int' is not initialized}} \
+ // expected-note {{subobject 'a' is not initialized}} \
  // ref-error {{must be initialized by a constant expression}} \
- // ref-note {{subobject of type 'int' is not initialized}}
+ // ref-note {{subobject 'a' is not initialized}}
 
 
   class Base {
@@ -161,18 +161,18 @@
 constexpr Derived() : Base() {}   };
 
   constexpr Derived D; // expected-error {{must be initialized by a constant expression}} \
-   // expected-note {{subobject of 

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:2357-2361
+if (SubobjectDecl) {
+  Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << 
SubobjectDecl;
+  Info.Note(SubobjectDecl->getLocation(),
+diag::note_constexpr_subobject_declared_here);
+}

tbaeder wrote:
> aaron.ballman wrote:
> > hazohelet wrote:
> > > aaron.ballman wrote:
> > > > Hmm, this breaks one of the contracts of our constexpr evaluation 
> > > > engine, doesn't it? My understanding is that if constexpr evaluation 
> > > > fails, we will have emitted a note diagnostic for why it failed. But if 
> > > > the caller doesn't pass a nonnull `SubobjectDecl`, we'll return `false` 
> > > > but we won't issue a diagnostic.
> > > > 
> > > > I'm surprised no tests lost notes as a result of this change, that 
> > > > suggests we're missing test coverage for the cases where nullptr is 
> > > > passed in explicitly to this function.
> > > Yeah, I was looking into when `SubobjectDecl` can be null here. I 
> > > `assert`ed the non-nullness of `SubobjectDecl` before and found that 
> > > there exists two lines of code 
> > > (https://github.com/llvm/llvm-project/blob/22a3f974d35da89247c0396594f2e4cd592742eb/clang/test/SemaCXX/attr-weak.cpp#L49
> > >  and 
> > > https://github.com/llvm/llvm-project/blob/abf4a8cb15d4faf04ee0da14e37d7349d3bde9a1/clang/test/CodeGenCXX/weak-external.cpp#L97)
> > >  in the test codes that nulls here.
> > > It seems they are doing the same thing, doing comparison against a 
> > > pointer to a `[[gnu::weak]]` member. A simple reproducing code is here: 
> > > https://godbolt.org/z/qn997n85n
> > > As you can see from the compiler explorer, there's no note emitted here 
> > > before the patch.
> > > I inserted some `printf` into the code before this patch  and confirmed 
> > > `Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << true << 
> > > Type` was actually called when compiling the reproducing code and that 
> > > somehow it is ignored. FWIW, `SubobjectLoc.isValid()` was `false` here.
> > > It seems they are doing the same thing, doing comparison against a 
> > > pointer to a [[gnu::weak]] member. A simple reproducing code is here: 
> > > https://godbolt.org/z/qn997n85n
> > > As you can see from the compiler explorer, there's no note emitted here 
> > > before the patch.
> > 
> > I see a note generated there:
> > ```
> > :4:41: note: comparison against pointer to weak member 
> > 'Weak::weak_method' can only be performed at runtime
> > constexpr bool val = ::weak_method != nullptr;
> > ^
> > ```
> > The situations I'm concerned about are the changes to ExprConstant.cpp:2270 
> > or line 2399 and so on.
> The `FFDiag` call can just stay as it was, right? And then only the 
> `Info.Note` call needs to be conditional for whether we have  a 
> `SubobjectDecl`.
In my understanding, the concern is that there could be note loss when 
`Value.hasValue()` evaluates to `false` and at the same time `SubobjectDecl` is 
`nullptr` explicitly passed in this change. 
`[[gnu::weak]]` member pointer comparison is the only example of this happening 
in the clang test codes where `Subobject` is `nullptr` passed at 
ExprConstant.cpp:2454 in `CheckFullyInitialized`. In this case the 
"uninitialized subobject" note was not displayed before this patch as well 
because there is another note issued elsewhere. Thus, the note loss does not 
happen.

BTW, I checked where the `[[gnu::weak]]` note is issued and I think there might 
be a mistake in the return statements. The `return true` should be `return 
false` because the constexpr evaluator fails to evaluate the expression, right? 
Correct me if I am wrong. Here's the reference:
https://github.com/llvm/llvm-project/blob/e58a49300e757ff61142f6abd227bd1437c1cf87/clang/lib/AST/ExprConstant.cpp#L13140-L13151

If we change them to `false`, `CheckFullyInitialized` will not be called in 
this scenario, and we will  have no test cases where `SubobjectDecl` is 
`nullptr` and at the same time `Value.hasValue()` is `false`. I am unsure 
whether this is due to the lack of test coverage or it is guaranteed that this 
condition does not hold here.

> The `FFDiag` call can just stay as it was, right? And then only the 
> `Info.Note` call needs to be conditional for whether we have  a 
> `SubobjectDecl`.
If `SubobjectDecl` is null, it would cause segfault when the `FFDiag` note is 
displayed, I think.

The best way I can think of would be to `assert(SubobjectDecl)` here, but I am 
still struggling and would like to ask for comments.


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

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

tbaeder added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:2357-2361
+if (SubobjectDecl) {
+  Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << 
SubobjectDecl;
+  Info.Note(SubobjectDecl->getLocation(),
+diag::note_constexpr_subobject_declared_here);
+}

aaron.ballman wrote:
> hazohelet wrote:
> > aaron.ballman wrote:
> > > Hmm, this breaks one of the contracts of our constexpr evaluation engine, 
> > > doesn't it? My understanding is that if constexpr evaluation fails, we 
> > > will have emitted a note diagnostic for why it failed. But if the caller 
> > > doesn't pass a nonnull `SubobjectDecl`, we'll return `false` but we won't 
> > > issue a diagnostic.
> > > 
> > > I'm surprised no tests lost notes as a result of this change, that 
> > > suggests we're missing test coverage for the cases where nullptr is 
> > > passed in explicitly to this function.
> > Yeah, I was looking into when `SubobjectDecl` can be null here. I 
> > `assert`ed the non-nullness of `SubobjectDecl` before and found that there 
> > exists two lines of code 
> > (https://github.com/llvm/llvm-project/blob/22a3f974d35da89247c0396594f2e4cd592742eb/clang/test/SemaCXX/attr-weak.cpp#L49
> >  and 
> > https://github.com/llvm/llvm-project/blob/abf4a8cb15d4faf04ee0da14e37d7349d3bde9a1/clang/test/CodeGenCXX/weak-external.cpp#L97)
> >  in the test codes that nulls here.
> > It seems they are doing the same thing, doing comparison against a pointer 
> > to a `[[gnu::weak]]` member. A simple reproducing code is here: 
> > https://godbolt.org/z/qn997n85n
> > As you can see from the compiler explorer, there's no note emitted here 
> > before the patch.
> > I inserted some `printf` into the code before this patch  and confirmed 
> > `Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << true << Type` 
> > was actually called when compiling the reproducing code and that somehow it 
> > is ignored. FWIW, `SubobjectLoc.isValid()` was `false` here.
> > It seems they are doing the same thing, doing comparison against a pointer 
> > to a [[gnu::weak]] member. A simple reproducing code is here: 
> > https://godbolt.org/z/qn997n85n
> > As you can see from the compiler explorer, there's no note emitted here 
> > before the patch.
> 
> I see a note generated there:
> ```
> :4:41: note: comparison against pointer to weak member 
> 'Weak::weak_method' can only be performed at runtime
> constexpr bool val = ::weak_method != nullptr;
> ^
> ```
> The situations I'm concerned about are the changes to ExprConstant.cpp:2270 
> or line 2399 and so on.
The `FFDiag` call can just stay as it was, right? And then only the `Info.Note` 
call needs to be conditional for whether we have  a `SubobjectDecl`.


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:2357-2361
+if (SubobjectDecl) {
+  Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << 
SubobjectDecl;
+  Info.Note(SubobjectDecl->getLocation(),
+diag::note_constexpr_subobject_declared_here);
+}

hazohelet wrote:
> aaron.ballman wrote:
> > Hmm, this breaks one of the contracts of our constexpr evaluation engine, 
> > doesn't it? My understanding is that if constexpr evaluation fails, we will 
> > have emitted a note diagnostic for why it failed. But if the caller doesn't 
> > pass a nonnull `SubobjectDecl`, we'll return `false` but we won't issue a 
> > diagnostic.
> > 
> > I'm surprised no tests lost notes as a result of this change, that suggests 
> > we're missing test coverage for the cases where nullptr is passed in 
> > explicitly to this function.
> Yeah, I was looking into when `SubobjectDecl` can be null here. I `assert`ed 
> the non-nullness of `SubobjectDecl` before and found that there exists two 
> lines of code 
> (https://github.com/llvm/llvm-project/blob/22a3f974d35da89247c0396594f2e4cd592742eb/clang/test/SemaCXX/attr-weak.cpp#L49
>  and 
> https://github.com/llvm/llvm-project/blob/abf4a8cb15d4faf04ee0da14e37d7349d3bde9a1/clang/test/CodeGenCXX/weak-external.cpp#L97)
>  in the test codes that nulls here.
> It seems they are doing the same thing, doing comparison against a pointer to 
> a `[[gnu::weak]]` member. A simple reproducing code is here: 
> https://godbolt.org/z/qn997n85n
> As you can see from the compiler explorer, there's no note emitted here 
> before the patch.
> I inserted some `printf` into the code before this patch  and confirmed 
> `Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << true << Type` 
> was actually called when compiling the reproducing code and that somehow it 
> is ignored. FWIW, `SubobjectLoc.isValid()` was `false` here.
> It seems they are doing the same thing, doing comparison against a pointer to 
> a [[gnu::weak]] member. A simple reproducing code is here: 
> https://godbolt.org/z/qn997n85n
> As you can see from the compiler explorer, there's no note emitted here 
> before the patch.

I see a note generated there:
```
:4:41: note: comparison against pointer to weak member 
'Weak::weak_method' can only be performed at runtime
constexpr bool val = ::weak_method != nullptr;
^
```
The situations I'm concerned about are the changes to ExprConstant.cpp:2270 or 
line 2399 and so on.


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet updated this revision to Diff 509686.
hazohelet added a comment.

Added release note


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

https://reviews.llvm.org/D146358

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/Interp.cpp
  clang/test/AST/Interp/cxx20.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -761,7 +761,7 @@
 };
 
 S s1; // expected-error {{call to consteval function 'NamespaceScopeConsteval::S::S' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 template 
 struct T {
@@ -770,7 +770,7 @@
 };
 
 T t; // expected-error {{call to consteval function 'NamespaceScopeConsteval::T::T' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 } // namespace NamespaceScopeConsteval
 
Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -409,12 +409,12 @@
 b.a.x = 2;
 return b;
   }
-  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
   static_assert(return_uninit().a.x == 2);
   constexpr A return_uninit_struct() {
 B b = {.b = 1};
 b.a.x = 2;
-return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject of type 'int' is not initialized}}
+return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject 'y' is not initialized}}
   }
   // Note that this is rejected even though return_uninit() is accepted, and
   // return_uninit() copies the same stuff wrapped in a union.
@@ -558,7 +558,7 @@
 }
   };
   constinit X x1(true);
-  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject of type 'int' is not initialized}}
+  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject 'n' is not initialized}}
 
   struct Y {
 struct Z { int n; }; // expected-note {{here}}
@@ -577,7 +577,7 @@
   };
   // FIXME: This is working around clang not implementing DR2026. With that
   // fixed, we should be able to test this without the injected copy.
-  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject 'n' is not initialized}}
   constexpr Y y1 = copy(Y());
   static_assert(y1.z1.n == 1 && y1.z2.n == 2 && y1.z3.n == 3);
 
Index: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
+++ clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
@@ -359,7 +359,7 @@
   // The constructor is still 'constexpr' here, but the result is not intended
   // to be a constant expression. The standard is not clear on how this should
   // work.
-  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
 
   constexpr int k = V().x; // FIXME: ok?
 }
Index: clang/test/AST/Interp/cxx20.cpp
===
--- clang/test/AST/Interp/cxx20.cpp
+++ clang/test/AST/Interp/cxx20.cpp
@@ -143,9 +143,9 @@
 constexpr A() {}
   };
   constexpr A a; // expected-error {{must be initialized by a constant expression}} \
- // expected-note {{subobject of type 'int' is not initialized}} \
+ // expected-note {{subobject 'a' is not initialized}} \
  // ref-error {{must be initialized by a constant expression}} \
- // ref-note {{subobject of type 'int' is not initialized}}
+ // ref-note {{subobject 'a' is not initialized}}
 
 
   class Base {
@@ -161,18 +161,18 @@
 constexpr Derived() : Base() {}   };
 
   constexpr Derived D; // expected-error {{must be initialized by a constant expression}} \
-   

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

@aaron.ballman 
Thanks for the review! I'll add a release note.




Comment at: clang/lib/AST/ExprConstant.cpp:2357-2361
+if (SubobjectDecl) {
+  Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << 
SubobjectDecl;
+  Info.Note(SubobjectDecl->getLocation(),
+diag::note_constexpr_subobject_declared_here);
+}

aaron.ballman wrote:
> Hmm, this breaks one of the contracts of our constexpr evaluation engine, 
> doesn't it? My understanding is that if constexpr evaluation fails, we will 
> have emitted a note diagnostic for why it failed. But if the caller doesn't 
> pass a nonnull `SubobjectDecl`, we'll return `false` but we won't issue a 
> diagnostic.
> 
> I'm surprised no tests lost notes as a result of this change, that suggests 
> we're missing test coverage for the cases where nullptr is passed in 
> explicitly to this function.
Yeah, I was looking into when `SubobjectDecl` can be null here. I `assert`ed 
the non-nullness of `SubobjectDecl` before and found that there exists two 
lines of code 
(https://github.com/llvm/llvm-project/blob/22a3f974d35da89247c0396594f2e4cd592742eb/clang/test/SemaCXX/attr-weak.cpp#L49
 and 
https://github.com/llvm/llvm-project/blob/abf4a8cb15d4faf04ee0da14e37d7349d3bde9a1/clang/test/CodeGenCXX/weak-external.cpp#L97)
 in the test codes that nulls here.
It seems they are doing the same thing, doing comparison against a pointer to a 
`[[gnu::weak]]` member. A simple reproducing code is here: 
https://godbolt.org/z/qn997n85n
As you can see from the compiler explorer, there's no note emitted here before 
the patch.
I inserted some `printf` into the code before this patch  and confirmed 
`Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << true << Type` was 
actually called when compiling the reproducing code and that somehow it is 
ignored. FWIW, `SubobjectLoc.isValid()` was `false` here.


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this! The changes should also come with a release note 
in `clang/docs/ReleaseNotes.rst` to note the improvement.




Comment at: clang/lib/AST/ExprConstant.cpp:2357-2361
+if (SubobjectDecl) {
+  Info.FFDiag(DiagLoc, diag::note_constexpr_uninitialized) << 
SubobjectDecl;
+  Info.Note(SubobjectDecl->getLocation(),
+diag::note_constexpr_subobject_declared_here);
+}

Hmm, this breaks one of the contracts of our constexpr evaluation engine, 
doesn't it? My understanding is that if constexpr evaluation fails, we will 
have emitted a note diagnostic for why it failed. But if the caller doesn't 
pass a nonnull `SubobjectDecl`, we'll return `false` but we won't issue a 
diagnostic.

I'm surprised no tests lost notes as a result of this change, that suggests 
we're missing test coverage for the cases where nullptr is passed in explicitly 
to this function.


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[suman meena via Phabricator via cfe-commits]

simideveloper added a comment.

ok, no problem and thanks for those further issues @hazohelet


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

In D146358#4230122 , @simideveloper 
wrote:

> hey, I was working on the same issue so can we collaborate on this issue?

@simideveloper Thanks for your interest in collaborating on this issue, and 
sorry if I discouraged you from contributing by working on the same problem.
I assume this patch is in the its final stage, awaiting feedback on wording. 
However, there's more room for improvement around the uninitialized subobject 
diagnostics of clang, and your experience working on this issue could be 
helpful in resolving those issues.
One example of further improvement:

  struct Type1 { int val; }; // note emitted
  
  struct Derived {
Type1 val; // we could note on this as well
constexpr Derived(){}
  };
  
  constexpr Derived D;

Link: https://godbolt.org/z/v3rhz88aM
I am open to future collaboration opportunities as well. Happy hacking!


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[suman meena via Phabricator via cfe-commits]

simideveloper added a comment.

hey, I was working on the same issue so can we collaborate on this issue?


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

In D146358#4229120 , @hazohelet wrote:

> In D146358#4227938 , @cjdb wrote:
>
>> In D146358#4204412 , @tbaeder 
>> wrote:
>>
>>> "subobject named 'foo'" sounds a bit weird to me, I'd expect just 
>>> "subobject 'foo'", but that's just a suggestion and I'll wait for a native 
>>> spearker to chime in on this.
>>
>> My expert brain likes `subobject of type 'T'`, but it's very wordy, and 
>> potentially not useful additional info. I'm partial to `subobject 'T'` due 
>> to it being the thing we're more likely to say in conversation (e.g. "that 
>> `int` isn't initialised"). I've also put out a mini-survey on the #include 
>>  Discord server, and will update in a few hours.
>
> Hi, @cjdb thanks for your feedback and the mini-survey.
> The objective of this patch is to print the subobject's name instead of its 
> type when it is not initialized in constexpr variable initializations. Thus, 
> I think it would be appropriate to add some more options to the survey like 
> "subobject named 'foo' is not initialized" and "subobject 'foo' is not 
> initialized" when the code looks like the following:
>
>   template 
>   struct F {
>   T foo;
>   constexpr F(){}
>   };
>   constexpr F f;

Ah, in that case, `subobject 'foo'` is better than `subobject named 'foo'` to 
me.


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

In D146358#4227938 , @cjdb wrote:

> In D146358#4204412 , @tbaeder wrote:
>
>> "subobject named 'foo'" sounds a bit weird to me, I'd expect just "subobject 
>> 'foo'", but that's just a suggestion and I'll wait for a native spearker to 
>> chime in on this.
>
> My expert brain likes `subobject of type 'T'`, but it's very wordy, and 
> potentially not useful additional info. I'm partial to `subobject 'T'` due to 
> it being the thing we're more likely to say in conversation (e.g. "that `int` 
> isn't initialised"). I've also put out a mini-survey on the #include  
> Discord server, and will update in a few hours.

Hi, @cjdb thanks for your feedback and the mini-survey.
The objective of this patch is to print the subobject's name instead of its 
type when it is not initialized in constexpr variable initializations. Thus, I 
think it would be appropriate to add some more options to the survey like 
"subobject named 'foo' is not initialized" and "subobject 'foo' is not 
initialized" when the code looks like the following:

  template 
  struct F {
T foo;
constexpr F(){}
  };
  constexpr F f;


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

In D146358#4204412 , @tbaeder wrote:

> "subobject named 'foo'" sounds a bit weird to me, I'd expect just "subobject 
> 'foo'", but that's just a suggestion and I'll wait for a native spearker to 
> chime in on this.

My expert brain likes `subobject of type 'T'`, but it's very wordy, and 
potentially not useful additional info. I'm partial to `subobject 'T'` due to 
it being the thing we're more likely to say in conversation (e.g. "that `int` 
isn't initialised"). I've also put out a mini-survey on the #include  
Discord server, and will update in a few hours.


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

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

tbaeder added a subscriber: cjdb.
tbaeder added a comment.

Pinging @cjdb and @aaron.ballman for some feedback on the wording


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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet updated this revision to Diff 506358.
hazohelet added a comment.

Address comments from @shafik and @tbaeder


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

https://reviews.llvm.org/D146358

Files:
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/Interp.cpp
  clang/test/AST/Interp/cxx20.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -761,7 +761,7 @@
 };
 
 S s1; // expected-error {{call to consteval function 'NamespaceScopeConsteval::S::S' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 template 
 struct T {
@@ -770,7 +770,7 @@
 };
 
 T t; // expected-error {{call to consteval function 'NamespaceScopeConsteval::T::T' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject 'Val' is not initialized}}
 
 } // namespace NamespaceScopeConsteval
 
Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -409,12 +409,12 @@
 b.a.x = 2;
 return b;
   }
-  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
   static_assert(return_uninit().a.x == 2);
   constexpr A return_uninit_struct() {
 B b = {.b = 1};
 b.a.x = 2;
-return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject of type 'int' is not initialized}}
+return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject 'y' is not initialized}}
   }
   // Note that this is rejected even though return_uninit() is accepted, and
   // return_uninit() copies the same stuff wrapped in a union.
@@ -558,7 +558,7 @@
 }
   };
   constinit X x1(true);
-  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject of type 'int' is not initialized}}
+  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject 'n' is not initialized}}
 
   struct Y {
 struct Z { int n; }; // expected-note {{here}}
@@ -577,7 +577,7 @@
   };
   // FIXME: This is working around clang not implementing DR2026. With that
   // fixed, we should be able to test this without the injected copy.
-  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject 'n' is not initialized}}
   constexpr Y y1 = copy(Y());
   static_assert(y1.z1.n == 1 && y1.z2.n == 2 && y1.z3.n == 3);
 
Index: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
+++ clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
@@ -359,7 +359,7 @@
   // The constructor is still 'constexpr' here, but the result is not intended
   // to be a constant expression. The standard is not clear on how this should
   // work.
-  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject 'y' is not initialized}}
 
   constexpr int k = V().x; // FIXME: ok?
 }
Index: clang/test/AST/Interp/cxx20.cpp
===
--- clang/test/AST/Interp/cxx20.cpp
+++ clang/test/AST/Interp/cxx20.cpp
@@ -143,9 +143,9 @@
 constexpr A() {}
   };
   constexpr A a; // expected-error {{must be initialized by a constant expression}} \
- // expected-note {{subobject of type 'int' is not initialized}} \
+ // expected-note {{subobject 'a' is not initialized}} \
  // ref-error {{must be initialized by a constant expression}} \
- // ref-note {{subobject of type 'int' is not initialized}}
+ // ref-note {{subobject 'a' is not initialized}}
 
 
   class Base {
@@ -161,18 +161,18 @@
 constexpr Derived() : Base() {}   };
 
   constexpr Derived D; // expected-error {{must be initialized by a constant expression}} \
- 

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

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

tbaeder added a comment.

"subobject named 'foo'" sounds a bit weird to me, I'd expect just "subobject 
'foo'", but that's just a suggestion and I'll wait for a native spearker to 
chime in on this.




Comment at: clang/lib/AST/Interp/Interp.cpp:373
+   const FieldDecl *SubObjDecl) {
+  S.FFDiag(SI, diag::note_constexpr_uninitialized) << SubObjDecl;
+  S.Note(SubObjDecl->getLocation(),

shafik wrote:
> shafik wrote:
> > Do we need to check that `SubObjDecl` is not `nullptr` ever?
> If we are sure it can never be `nullptr` then we should `assert(SubObjDecl)`
Should never be null here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/Interp/Interp.cpp:373
+   const FieldDecl *SubObjDecl) {
+  S.FFDiag(SI, diag::note_constexpr_uninitialized) << SubObjDecl;
+  S.Note(SubObjDecl->getLocation(),

shafik wrote:
> Do we need to check that `SubObjDecl` is not `nullptr` ever?
If we are sure it can never be `nullptr` then we should `assert(SubObjDecl)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Thank you for submitting this fix.




Comment at: clang/lib/AST/ExprConstant.cpp:2270
+ Info, MTE->getExprLoc(), TempType, *V, Kind,
+ nullptr, CheckedTemps))
 return false;

To conform to [clang-tidy bugprone-argument-comment 
format](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)



Comment at: clang/lib/AST/ExprConstant.cpp:2399
Value.getStructBase(BaseIndex), Kind,
-   BS.getBeginLoc(), CheckedTemps))
+   nullptr, CheckedTemps))
   return false;

same here.



Comment at: clang/lib/AST/ExprConstant.cpp:2443
   return CheckEvaluationResult(CheckEvaluationResultKind::ConstantExpression,
-   Info, DiagLoc, Type, Value, Kind,
-   SourceLocation(), CheckedTemps);
+   Info, DiagLoc, Type, Value, Kind, nullptr,
+   CheckedTemps);

Also here



Comment at: clang/lib/AST/ExprConstant.cpp:2454
+   Info, DiagLoc, Type, Value,
+   ConstantExprKind::Normal, nullptr, 
CheckedTemps);
 }

and here



Comment at: clang/lib/AST/Interp/Interp.cpp:373
+   const FieldDecl *SubObjDecl) {
+  S.FFDiag(SI, diag::note_constexpr_uninitialized) << SubObjDecl;
+  S.Note(SubObjDecl->getLocation(),

Do we need to check that `SubObjDecl` is not `nullptr` ever?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146358

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146358: [clang][AST] Print name instead of type when diagnosing uninitialized subobject in constexpr variables

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet created this revision.
hazohelet added reviewers: aaron.ballman, tbaeder, shafik.
Herald added a project: All.
hazohelet requested review of this revision.
Herald added a project: clang.

This patch improves the diagnostic on uninitialized subobjects in constexpr 
variables by modifying the diagnostic message to display the subobject's name 
instead of its type.

Fixes https://github.com/llvm/llvm-project/issues/58601


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146358

Files:
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Interp/Interp.cpp
  clang/test/AST/Interp/cxx20.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -761,7 +761,7 @@
 };
 
 S s1; // expected-error {{call to consteval function 'NamespaceScopeConsteval::S::S' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject named 'Val' is not initialized}}
 
 template 
 struct T {
@@ -770,7 +770,7 @@
 };
 
 T t; // expected-error {{call to consteval function 'NamespaceScopeConsteval::T::T' is not a constant expression}} \
- expected-note {{subobject of type 'int' is not initialized}}
+ expected-note {{subobject named 'Val' is not initialized}}
 
 } // namespace NamespaceScopeConsteval
 
Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -409,12 +409,12 @@
 b.a.x = 2;
 return b;
   }
-  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr B uninit = return_uninit(); // expected-error {{constant expression}} expected-note {{subobject named 'y' is not initialized}}
   static_assert(return_uninit().a.x == 2);
   constexpr A return_uninit_struct() {
 B b = {.b = 1};
 b.a.x = 2;
-return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject of type 'int' is not initialized}}
+return b.a; // expected-note {{in call to 'A(b.a)'}} expected-note {{subobject named 'y' is not initialized}}
   }
   // Note that this is rejected even though return_uninit() is accepted, and
   // return_uninit() copies the same stuff wrapped in a union.
@@ -558,7 +558,7 @@
 }
   };
   constinit X x1(true);
-  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject of type 'int' is not initialized}}
+  constinit X x2(false); // expected-error {{constant initializer}} expected-note {{constinit}} expected-note {{subobject named 'n' is not initialized}}
 
   struct Y {
 struct Z { int n; }; // expected-note {{here}}
@@ -577,7 +577,7 @@
   };
   // FIXME: This is working around clang not implementing DR2026. With that
   // fixed, we should be able to test this without the injected copy.
-  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr Y copy(Y y) { return y; } // expected-note {{in call to 'Y(y)'}} expected-note {{subobject named 'n' is not initialized}}
   constexpr Y y1 = copy(Y());
   static_assert(y1.z1.n == 1 && y1.z2.n == 2 && y1.z3.n == 3);
 
Index: clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
===
--- clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
+++ clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p4.cpp
@@ -359,7 +359,7 @@
   // The constructor is still 'constexpr' here, but the result is not intended
   // to be a constant expression. The standard is not clear on how this should
   // work.
-  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject of type 'int' is not initialized}}
+  constexpr V v; // expected-error {{constant expression}} expected-note {{subobject named 'y' is not initialized}}
 
   constexpr int k = V().x; // FIXME: ok?
 }
Index: clang/test/AST/Interp/cxx20.cpp
===
--- clang/test/AST/Interp/cxx20.cpp
+++ clang/test/AST/Interp/cxx20.cpp
@@ -143,9 +143,9 @@
 constexpr A() {}
   };
   constexpr A a; // expected-error {{must be initialized by a constant expression}} \
- // expected-note {{subobject of type 'int' is not initialized}} \
+ // expected-note {{subobject named 'a' is not initialized}} \
  // ref-error {{must be initialized by a constant expression}} \
-