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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD marked 4 inline comments as done.
RedDocMD 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 ||
----------------
xazax.hun wrote:
> 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.)
Yes that's the next patch. (and one more for `std::hash`).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:341
+      case OO_Spaceship:
+        // TODO: What would be a good thing to do here?
+      default:
----------------
xazax.hun wrote:
> 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.
Ah yes that's what is happening now. `RetVal` is initialized to a conjured value. If we can conclude no further, then that is what is returned - which is what happens here. In other cases, constraints are applied or it is replaced by the output from `evalBinOp`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:354
+
+      if (FirstPtr && !SecondPtr &&
+          State->assume(FirstPtr->castAs<DefinedOrUnknownSVal>(), false)) {
----------------
xazax.hun wrote:
> 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.
Actually the naming is a bit misleading here, perhaps. `FirstPtr` is not the inner pointer itself but a pointer to the //SVal//. So the test `FirstPtr && !SecondPtr` checks that we know the SVal for the inner pointer of the first arg but we do not know that of the second arg. Using a null-pointer constant would not help.
However, we could use a conjured value to simplify some work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616