[PATCH] D105127: Implement P1401R5

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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">;
----------------
rsmith wrote:
> mizvekov wrote:
> > cor3ntin wrote:
> > > 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
> > I see, yeah agreed.
> Would it be reasonable to drop the "convertible to bool" part here? We know the problem is that the (converted) expression is not a constant expression, not that the expression can't be converted to bool, because we handle the conversion to bool separately before we get to this diagnostic; I think the diagnostic would be clearer if it didn't mention the conversion.
+1 -- I think the "convertible to bool" may trip some users up on how to correct the issue.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:3925
+  // is contextually converted to bool and the converted expression shall be
+  // a constant expression
+  //
----------------



================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:3930
+  ExprResult E = PerformContextuallyConvertToBool(CondExpr);
+  if (!IsConstexpr || !E.isUsable() || E.get()->isValueDependent())
+    return E;
----------------



================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:3922
   //
   // FIXME: Return this value to the caller so they don't need to recompute it.
+
----------------
mizvekov wrote:
> This FIXME got pushed out of the thing it used to refer to.
+1, I think this comment should move down to the `APSInt` declaration so it's clear which value we want returned to the caller someday.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105127