rsmith added inline comments.
================
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) {
----------------
vsapsai wrote:
> 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.
Would renaming this to something like
`shouldEvaluateForUndefinedBehaviorChecks` help? That still doesn't completely
capture the non-recursive nature. I think improving the comment is a good idea
regardless.
================
Comment at: clang/lib/AST/ExprConstant.cpp:8398-8399
bool LValueExprEvaluator::VisitUnaryPreIncDec(const UnaryOperator *UO) {
- if (!Info.getLangOpts().CPlusPlus14 && !Info.keepEvaluatingAfterFailure())
- return Error(UO);
----------------
vsapsai wrote:
> 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`.
Yes, exactly.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:7125-7126
<< DestTy;
- return CK_IntegralCast;
+ Src = ExprError();
+ return CK_Dependent;
case Type::STK_CPointer:
----------------
vsapsai wrote:
> 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`.
It shouldn't matter what we return, because the caller should always bail out
and ignore our return value if `Src` is set to an invalid expression. But in
principle, `CK_IntegralCast` is wrong, because this situation doesn't meet the
constraints for an integral cast. We use "dependent" to mean not only
"dependent on a C++ template parameter" but also "dependent on an error", so
that seemed like the least wrong cast kind for this situation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98912/new/
https://reviews.llvm.org/D98912
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
