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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

F17741480: 704.jpg <https://reviews.llvm.org/F17741480>

I only have a few nits left.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:35
 #include <string>
+#include <variant>
 
----------------
I think you're not using this anymore.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:316
+
+class OperatorKind {
+  union {
----------------
One good place to put this may be `CheckerHelpers.h`. This is where we dump all the stuff that's probably useful in multiple checkers but not in other places.

I also wonder if you plan to support unary operators. The interesting part about them is that they are sometimes ambiguous to binary operators in their string representation, eg. `-`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:435
+      operationKindFromOverloadedOperator(OOK).GetBinaryOpUnsafe();
+  auto RetVal = C.getSValBuilder().evalBinOp(
+      State, BOK, FirstPtrVal, SecondPtrVal, Call.getResultType());
----------------
`C.getSValBuilder()` is called three times now, you might want to stash it in a reference. There's probably not much performance benefit but the code should be easier to read this way.


================
Comment at: clang/test/Analysis/Inputs/system-header-simulator-cxx.h:983
+template <typename T1, typename T2>
+bool operator==(const unique_ptr<T1> &x, const unique_ptr<T2> &y);
+
----------------
I think it's a good idea to test these cross-type comparison operators as well. IIUC they're handled exactly the same as their same-type counterparts but it may uncover occasional assertion failures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616