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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408
+  auto makeSValFor = [&C, &State, this](const Expr *E, SVal S) -> SVal {
+    if (S.isZeroConstant()) {
+      return C.getSValBuilder().makeZeroVal(E->getType());
----------------
Do we want to create a new SVal in this case? Why returning `S` is insufficient?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:416
+    QualType Type = getInnerPointerType(C, E->getType()->getAsCXXRecordDecl());
+    std::tie(Val, State) = retrieveOrConjureInnerPtrVal(Reg, E, Type, C);
+    return Val;
----------------
While this is smart (overwriting the state here), I think this is hard to follow. I'd prefer this lambda returning a pair.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:435
+    ProgramStateRef TrueState, FalseState;
+    std::tie(TrueState, FalseState) =
+        State->assume(*RetVal.getAs<DefinedOrUnknownSVal>());
----------------
`assume` might not return a state (when the analyzer is smart enough to figure out one path is infeasible). While your code would probably work as is, as far as I remember we usually try to not call addTransition with `null` states. 


================
Comment at: clang/test/Analysis/smart-ptr.cpp:484
+  clang_analyzer_eval(nullPtr != nullptr);    // expected-warning{{FALSE}}
+  clang_analyzer_eval(nullptr <= unknownPtr); // expected-warning{{TRUE}}
+}
----------------
I do not see test cases where the path is actually split. Would you add some?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616