[PATCH] D83286: [analyzer][solver] Track symbol disequalities

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

This looks great to me but i'd very much rather have someone else have a look, just because of the nature of the problem (:



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1586
+  // 4. Update disequality relations
+  if (const ClassSet *DisequalToOther = DisequalityInfo.lookup(Other)) {
+    ClassSet DisequalToThis = getDisequalClasses(State);
----------------
Nit: I think it's a bit messy to use `getDisequalClasses()` in some places but manual lookup in other places. Like, someone may change the function and believe that they changed the entire lookup mechanism but fail to notice that the function wasn't used consitently. I think this may be worth encapsulating.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1727
+
+  // Let's check if know anything about these two classes being not equal to
+  // each other.
----------------
Parse error; missing "we"?


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1913-1915
+        // DisequalToDisequalSet is guaranteed to be non-null for consistent
+        // disequality info.
+        ClassSet NewSet = ClassSetFactory.remove(*DisequalToDisequalSet, Class);
----------------
I actually wouldn't mind duplicating a direct assertion here, given that consistency checks are pretty complicated on their own :)


================
Comment at: clang/test/Analysis/mutually_exclusive_null_fp.cpp:25
+
+  return compare(*aData, *bData);
+}
----------------
`// no-warning` please? These marks make tests a lot easier to understand when you break them 5 years later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83286