[PATCH] D105127: Implement P1401R5

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

Thanks!
Just some nits and some minor points on the tests.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1491
+def err_constexpr_if_condition_expression_is_not_constant : Error<
+  "constexpr if condition is not a constant expression convertible to bool">;
 def err_static_assert_failed : Error<"static_assert failed%select{ %1|}0">;
----------------
Looks a bit easier to parse the english there.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:3922
   //
   // FIXME: Return this value to the caller so they don't need to recompute it.
+
----------------
This FIXME got pushed out of the thing it used to refer to.


================
Comment at: clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp:1-4
 // RUN: %clang_cc1 -std=c++1z -verify %s
 // RUN: %clang_cc1 -std=c++1z -verify %s -DUNDEFINED
+// RUN: %clang_cc1 -std=c++2b -verify %s
+// RUN: %clang_cc1 -std=c++2b -verify %s -DUNDEFINED
----------------
I think attaching the expected diagnostics to the lines that generate them looks clearer and is easier to maintain.

The problem is that it seems you are having to work around that some of these errors are conditional on the std version, and it would indeed be quite a bite noisier with the preprocessor there, but there is a simple solution: with the suggested edit above, you can do things like:
````
// cxx17-error {{value of type 'ccce::S' is not implicitly convertible to 'bool'}}
// cxx2b-error {{value of type 'ccce::S' is not contextually convertible to 'bool'}}
````
But you can still do `expected-note {{cannot be used in a constant expression}}` in order to expect diagnostics for all versions.


================
Comment at: clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp:43
 namespace ccce {
+struct S {
+} s;
----------------
This is not consistently indented.


================
Comment at: clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp:71-72
+
+    constexpr S constexprS;
+    if constexpr (s) {
+    }
----------------
Hmm what is this testing exactly? That `constexprS` is not getting confused with `s`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105127