[PATCH] D104616: [analyzer][WIP] 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:301
+  const OverloadedOperatorKind OOK = FD->getOverloadedOperator();
+  if (!(OOK == OO_Equal || OOK == OO_ExclaimEqual || OOK == OO_Less ||
+        OOK == OO_LessEqual || OOK == OO_Greater || OOK == OO_GreaterEqual ||
----------------
So it looks like `operator<<` is the only operator we are not supporting. I think it might be good to include some test code to ensure that its use does not suppress warnings. (Also OK, if you decide to deal with this in a follow-up PR.)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:334
+      case OO_LessEqual:
+        State = State->assume((&RetVal)->castAs<DefinedOrUnknownSVal>(), true);
+        break;
----------------
I think in cases where we know that the result is `true` or `false`, the `RetVal` should probably be a constant instead of a conjured symbol with an assumption. 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:341
+      case OO_Spaceship:
+        // TODO: What would be a good thing to do here?
+      default:
----------------
Instead of marking this unreachable, I'd suggest to just return a conjured symbol for now.  Usually, we should not use asserts to test user input.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:354
+
+      if (FirstPtr && !SecondPtr &&
+          State->assume(FirstPtr->castAs<DefinedOrUnknownSVal>(), false)) {
----------------
I am not sure if we actually need all this special casing. You could create an `SVal` that represents a nullpointer constant and use `evalBinOp` with that `SVal` when there is no symbol available.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:394
+        default:
+          llvm_unreachable("cannot reach here");
+        }
----------------
`cannot reach here` is redundant information. That is already encoded in `llvm_unreachable`. I suggest including a message that tells the reader **why** is it unreachable. In this case it could be `"unexpected overloaded operator kind"`.


================
Comment at: clang/test/Analysis/smart-ptr.cpp:466
+
+  clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > ptr);  // expected-warning{{FALSE}}
----------------
Putting tests like this on the same path can be risky. Tests might split the execution path (maybe not now, but in the future). I think it might be more future proof to have a large switch statement that switches on an unknown value and put the tests in separate cases. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616