[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like keys

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Minor comments in-line, looking good on first glance.

I'm not sure if we discussed, has this checker been tried on some in-the-wild examples? To see if there are some real findings or crashes?
There is a good selection of projects here in this tool: https://github.com/Xazax-hun/csa-testbench.



================
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:46
+};
+}
+
----------------
Szelethus wrote:
> ` // end of anonymous namespace`
This comment can be marked as //Done//.


================
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:52
+  llvm::raw_string_ostream OS(Diagnostics);
+  OS << "Sorting pointer-like keys can result in non-deterministic ordering";
+
----------------
I generally have a concern about calling these thing "keys". How did you mean this? The file's top comment says //elements//.


================
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:81-87
+                         callsName("std::is_sorted"),
+                         callsName("std::nth_element"),
+                         callsName("std::partial_sort"),
+                         callsName("std::sort"),
+                         callsName("std::stable_sort"),
+                         callsName("std::partition"),
+                         callsName("std::stable_partition")
----------------
Please order this listing.


================
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:102
+  auto Matches = match(MatcherM, *D, AM.getASTContext());
+  for (BoundNodes Match : Matches)
+    emitDiagnostics(Match, D, BR, AM, this);
----------------
`emitDiagnostics` takes a `const T&`. To prevent unnecessary copy operations in this iteration, you should also use `const BoundNode &Match`.


https://reviews.llvm.org/D50488