ymandel marked 6 inline comments as done and an inline comment as not done.
ymandel added a comment.
Thanks for the detailed review!
================
Comment at:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:119
+ auto NonEmptyStringOptional = unaryOperator(
+ hasOperatorName("!"),
+ hasUnaryOperand(cxxMemberCallExpr(
----------------
sgatev wrote:
> Why handle negation here? Would it work for `if (opt.value_or("").empty()) {
> ... } else { opt.value(); }`?
The negation is a simpler predicate, but you're right that it's too specific.
I've rewritten the code to drop the constraint and handle the more general
`opt.value_or("").empty()`. The new code also encodes a more precise
relationship in the logic, per Gabor's comments below about the implication in
the other direction. It also now directly attaches the formula to the value,
rather than dropping an implication in the flow conditions.
Overall, I think the new approach is an improvement, but please let me know if
you disagree.
================
Comment at:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:148
+ anyOf(ComparesToSame(cxxNullPtrLiteralExpr()),
+
ComparesToSame(integerLiteral(equals(0)))))));
+}
----------------
sgatev wrote:
> Why `0`? How about `opt_p->value_or(21) != 21`?
This is addressed in the comment, but do you think we should add a FIXME to
support some amount of expression comparision? Integers, floats, bools and
variables would be an easy place to start, for example. But, we'd need to drop
into regular code -- the matchers can't express that kind of constraint.
================
Comment at:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:269
+ // the implication `(opt.value_or(X) != X) => opt.hasValue()`.
+ State.Env.addToFlowCondition(
+ State.Env.makeImplication(*ComparisonExprValue, *HasValueVal));
----------------
xazax.hun wrote:
> There is an implication in the reverse direction as well. In case we know the
> optional is empty, we can prune one of the branches from the analysis. Is it
> possible to implement that with the current status of the framework?
Yes, good point! Please see my response to Stanislav above. I think the new
version handles this by modeling the value_or directly, rather than dropping in
an implication.
================
Comment at:
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:482
+ .CaseOf<Expr>(
+ isValueOrCondition("ValueOrCall"),
+ [](const clang::Expr *E, const MatchFinder::MatchResult &Result,
----------------
sgatev wrote:
> Why not hard-code this in the `isValueOrCondition` matcher?
Safety/hygiene. It's easier to see that the ID to which the node is bound is
the same that's being used in `getNodeAs`. An alternative which I often use is
to use a (static) global constant, so I've changed to that.
================
Comment at:
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:1722
+ if (opt.value_or(nullptr) != nullptr) {
+ return *opt;
+ /*[[check-ptrs-1]]*/
----------------
sgatev wrote:
> Is the `return` important? I think having `void` return type would be
> simpler. Same comment for the cases below.
Totally. Thanks for pointing that out.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122231/new/
https://reviews.llvm.org/D122231
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
