NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:351-352
+ case OO_##Name:
\
+ BinOpIt = BinOps.find(Spelling);
\
+ UnOpIt = UnOps.find(Spelling);
\
+ if (BinOpIt != BinOps.end())
\
----------------
Optimization: if one of the lookups succeeds, the other is unnecessary. A bit
premature but I think we shouldn't lose too much elegance over this.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:388-389
+
+ auto retrieveOrConjureInnerPtrVal = [&C, &State](const Expr *E,
+ SVal S) -> SVal {
+ if (S.isZeroConstant()) {
----------------
This sounds like a super common functionality. Doesn't `.get()` do the same as
well? I think it should be de-duplicated into a top-level method.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:443-446
+ auto RetVal = C.getSValBuilder().evalBinOp(
+ State, BOK, FirstPtrVal, SecondPtrVal, Call.getResultType());
+ State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(),
RetVal);
+ C.addTransition(State);
----------------
Because these operators are pure boolean functions, a state split would be
justified. It's pointless to call the operator unless both return values are
feasible. I think you should do an `assume()` and transition into both states.
It might also make sense to consult `-analyzer-config eagerly-assume=` before
doing it but it sounds like this option will stays true forever and we might as
well remove it.
The spaceship operator is obviously an exceptional case. Invocation of the
spaceship operator isn't a good indication that all three return values are
feasible, might be only two.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104616/new/
https://reviews.llvm.org/D104616
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
