[PATCH] D62688: [Analyzer] Iterator Checkers - Model `empty()` method of containers

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 3 inline comments as done.
baloghadamsoftware added a comment.

In D62688#1549574 <https://reviews.llvm.org/D62688#1549574>, @Szelethus wrote:

> Hmm, an idea just popped into my head. I'm not sure whether we have a single checker that does so much complicated (and totally awesome) modeling as `IteratorChecker`.  What do you think about a debug checker similar to `debug.ExprInspection`, like `debug.IteratorInspection`?


Good idea! However, I would do it in a way that we can reuse our existing debug functions in the tests:

  template <typename Iter> long clang_iterator_position(const Iter&);
  template <typename Iter, typename Cont> const Cont& clang_iterator_container(const Iter&);
  template <typename Iter> long clang_container_begin(const Iter&);
  template <typename Iter> long clang_container_end(const Iter&);

Then we can nest calls for these functions into call of `clang_analyzer_dump()`, `clang_analyzer_eval()`, `clang_analyzer_denote()` etc.



================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:714
 
     if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) {
       if (isBeginCall(Func)) {
----------------
Szelethus wrote:
> We seem to retrieve the `CXXInstanceCall` here too?
Eventually this function needs refactoring.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1741-1763
+bool isContainer(const CXXRecordDecl *CRD) {
+  if (!CRD)
+    return false;
+
+  for (const auto *Decl : CRD->decls()) {
+    const auto *TD = dyn_cast<TypeDecl>(Decl);
+    if (!TD)
----------------
Szelethus wrote:
> But what if we have a container for which the iterator isn't an inline class, or we don't even have an in-class alias for it? I think whether the record has a `begin`/`end` method would be a better heuristic.
Or maybe both, or maybe either. I am not sure here.


================
Comment at: test/Analysis/iterator-range.cpp:247
+void empty(const std::vector<int> &V) {
+  for (auto n: V) {}
+  *V.begin(); // expected-warning{{Past-the-end iterator dereferenced}}
----------------
Szelethus wrote:
> Aha, the for loop is here to force a state split on whether `V` is empty or not, correct?
Yes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62688