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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

I'm only a beginner, but here are some things that caught my eye. I really like the idea! :)



================
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:28
+
+// PointerSortingVisitor class.
+class PointerSortingVisitor : public StmtVisitor<PointerSortingVisitor> {
----------------
This comment holds little value.


================
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:46
+};
+}
+
----------------
` // end of anonymous namespace`


================
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:88
+
+  if (!II->getName().equals("sort"))
+    return;
----------------
whisperity wrote:
> Brrr... `equals`. StringRef has a `==` and `!=` operator which accepts string literals on the other side, which would result in a more concise code.
> 
> Also, this heuristic can be applied without extra changes (apart from those mentioned by NoQ and might be mentioned later by others) to other sorting functions, such as `std::stable_sort` and `std::stable_partition`. Perhaps it would be worthy to enable checking those functions too.
Maybe `II->isStr("sort")`?


================
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:93
+  const RecordDecl *RD =
+    IterTy->getUnqualifiedDesugaredType()->getAsCXXRecordDecl();
+
----------------
Is there a reason for not directly acquiring the record declaration?


================
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:119
+};
+}
+
----------------
` // end of anonymous namespace`


================
Comment at: test/Analysis/ptr-sort.cpp:1
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.nondeterminism.PointerSorting %s -analyzer-output=text -verify
+
----------------
Always run the core checkers too.
`-analyzer-checker=core,alpha.nondeterminism.PointerSorting`


Repository:
  rC Clang

https://reviews.llvm.org/D50488