[PATCH] D105127: Implement P1401R5

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.


================
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">;
----------------
mizvekov wrote:
> Looks a bit easier to parse the english there.
I would rather not change that, to remain consistent with existing diagnostics involving `constexpr if`
But I agree it might be good to change them all


================
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
----------------
mizvekov wrote:
> 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.
Thanks, much better, I did not know it was an option


================
Comment at: clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp:43
 namespace ccce {
+struct S {
+} s;
----------------
mizvekov wrote:
> This is not consistently indented.
Unfortunately, it was put there by clang-format, should I move it manually?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105127