[PATCH] D122231: [clang][dataflow] Add support for `value_or` in a comparison.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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