[PATCH] D105127: Implement P1401R5

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:3930
+  ExprResult E = PerformContextuallyConvertToBool(CondExpr);
+  if (!IsConstexpr || CondExpr->isValueDependent())
+    return E;
----------------
Minor nit: I think it would be a tiny bit clearer if you factored out this condition, since you repeat it:
```
bool IsConstexprNonValueDependent = IsConstexpr  && !CondExpr->isValueDependent();
if (!LangOpts.CPlusPlus2b && IsConstexprNonValueDependent) {
....
if (!IsConstexprNonValueDependent)
```


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:3935
+  E = VerifyIntegerConstantExpression(
+      E.get(), &Cond,
+      diag::err_constexpr_if_condition_expression_is_not_constant);
----------------
The value returned from `PerformContextuallyConvertToBool` can have a possibly failed state (We follow this convention for FooResult named types).
But here there is a `get` on it on a path where there could be an error.
Can you handle this, and also add test case to cover it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105127