vsapsai accepted this revision.
vsapsai added inline comments.
This revision is now accepted and ready to land.
================
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) {
----------------
rsmith wrote:
> 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.
Don't have strong feelings about `shouldEvaluateForUndefinedBehaviorChecks`.
Both `shouldEvaluateForUndefinedBehaviorChecks` and
`mightWarnOnUndefinedBehavior` convey enough intention and actual
implementation is fairly easy to understand. And information at the call site
in `ShouldEvaluate` seems to be sufficient for intention and implementation
purposes.
Also considered inlining `mightWarnOnUndefinedBehavior` but it doesn't look
like a better alternative.
It's up to you if you want to tweak it further, I don't have good suggestions.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:7125-7126
<< DestTy;
- return CK_IntegralCast;
+ Src = ExprError();
+ return CK_Dependent;
case Type::STK_CPointer:
----------------
rsmith wrote:
> 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.
Thanks for the explanation. "dependent on an error" is an important piece of
information and now it makes more sense.
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
