[PATCH] D104616: [analyzer][WIP] Model comparision methods of std::unique_ptr

[Artem Dergachev via Phabricator via cfe-commits]

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