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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
