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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
