vsapsai added a comment.
Looks good to me. Have a few clarification questions.
================
Comment at: clang/lib/AST/ExprConstant.cpp:972-973
+ // Determine if we might warn that the given expression exhibits undefined
+ // behavior.
+ bool mightWarnOnUndefinedBehavior(const Expr *E) {
----------------
I'm not entirely sure but it can be useful to make it explicit in the comment
the method doesn't deal with all the subexpressions recursively. Not convinced
it is useful because don't have a good wording and you can get that information
from the short method implementation. It is possible my suggestion is caused by
checking the patch top-to-bottom without seeing how
`mightWarnOnUndefinedBehavior` is used first, but that's not how developers
would read the code later.
================
Comment at: clang/lib/AST/ExprConstant.cpp:8398-8399
bool LValueExprEvaluator::VisitUnaryPreIncDec(const UnaryOperator *UO) {
- if (!Info.getLangOpts().CPlusPlus14 && !Info.keepEvaluatingAfterFailure())
- return Error(UO);
----------------
Just to confirm my understanding. There is no point in checking
`keepEvaluatingAfterFailure` here because we are not short-circuiting anymore
but can evaluate the expression in `VisitExpr(UO)`. And we still check
`keepEvaluatingAfterFailure` in `SubexpressionVisitor::Run` in
`EvaluateForDiagnostics`.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:7125-7126
<< DestTy;
- return CK_IntegralCast;
+ Src = ExprError();
+ return CK_Dependent;
case Type::STK_CPointer:
----------------
What is the purpose of these changes to return `CK_Dependent`? Tests are
passing without them and it's not obvious to me why do you need to change
`CK_IntegralCast` to `CK_Dependent`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98912/new/
https://reviews.llvm.org/D98912
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
