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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added inline comments.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:119
+  auto NonEmptyStringOptional = unaryOperator(
+      hasOperatorName("!"),
+      hasUnaryOperand(cxxMemberCallExpr(
----------------
Why handle negation here? Would it work for `if (opt.value_or("").empty()) { ... } else { opt.value(); }`?


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:148
+                            anyOf(ComparesToSame(cxxNullPtrLiteralExpr()),
+                                  ComparesToSame(integerLiteral(equals(0)))))));
+}
----------------
Why `0`? How about `opt_p->value_or(21) != 21`?


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:480
 
+      // opt.value_or(X) != X, !opt.value_or("").empty():
+      .CaseOf<Expr>(
----------------
Extreme nit for consistency with all comments above.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:482
+      .CaseOf<Expr>(
+          isValueOrCondition("ValueOrCall"),
+          [](const clang::Expr *E, const MatchFinder::MatchResult &Result,
----------------
Why not hard-code this in the `isValueOrCondition` matcher?


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:483
+          isValueOrCondition("ValueOrCall"),
+          [](const clang::Expr *E, const MatchFinder::MatchResult &Result,
+             LatticeTransferState &State) {
----------------
The `clang` namespace can be removed. Same comment for other instances in the patch.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:485
+             LatticeTransferState &State) {
+            transferOptionalValueOrCall(
+                E,
----------------
Why not pass `transferOptionalValueOrCall` as argument instead of wrapping it in a lambda? The function can take the "ValueOrCall" node from the `MatchResult`.


================
Comment at: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:1722
+      if (opt.value_or(nullptr) != nullptr) {
+        return *opt;
+        /*[[check-ptrs-1]]*/
----------------
Is the `return` important? I think having `void` return type would be simpler. Same comment for the cases below.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122231/new/

https://reviews.llvm.org/D122231