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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Looks like a solid start!



================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:301
+
+  bool addTransition = false;
+  ProgramStateRef State = C.getState();
----------------
nit: variable names should be capitalized


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:321-323
+        State =
+            State->assume(llvm::dyn_cast<DefinedOrUnknownSVal>(RetVal), true);
+        addTransition = true;
----------------
Another idea here.
Instead of repeating:
```
State = State->assume(llvm::dyn_cast<DefinedOrUnknownSVal>(RetVal), VALUE);
addTransition = true;
```
and having boolean `addTransition`, you can have `Optional<bool> CompResult` and do:
```
CompResult = VALUE;
```
And do the actual assumption at the bottom section when `CompResult.hasValue()`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:334
+      default:
+        assert(false && "shouldn't reach here");
+      }
----------------
nit: `llvm_unreachable` instead of `assert(false)`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:345-364
+      if (FirstPtr && !SecondPtr &&
+          State->assume(llvm::dyn_cast<DefinedOrUnknownSVal>(*FirstPtr),
+                        false)) {
+        // FirstPtr is null, SecondPtr is unknown
+        if (OOK == OO_LessEqual) {
+          State =
+              State->assume(llvm::dyn_cast<DefinedOrUnknownSVal>(RetVal), true);
----------------
These are also symmetrical.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:373-391
+          switch (OOK) {
+          case OO_Equal:
+          case OO_GreaterEqual:
+          case OO_LessEqual:
+            State = State->assume(llvm::dyn_cast<DefinedOrUnknownSVal>(RetVal),
+                                  true);
+            addTransition = true;
----------------
This switch exactly repeats the previous switch.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:394-436
+        if (FirstNull && !SecondNull) {
+          switch (OOK) {
+          case OO_Less:
+          case OO_LessEqual:
+            State = State->assume(llvm::dyn_cast<DefinedOrUnknownSVal>(RetVal),
+                                  true);
+            addTransition = true;
----------------
These are symmetrical, is there a way to implement it without this boilerplate?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616