[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

[Anna Zaks via Phabricator via cfe-commits]

zaks.anna added a comment.

It's awesome to see that all the major issues have been addressed. Thank you for working on this and diligently working through the code review!!!

I have a few minor comments below.

Could you add this example yours as a "no-warning" test case:
const auto start = v.begin();
while(true) {

  const auto item = find(start, v.end());
  if(item==v.end())
     break;
  doSomething(*item);
  start = ++item;

}



================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:166
+IteratorPastEndChecker::IteratorPastEndChecker() {
+  PastEndBugType.reset(new BugType(this, "Iterator Past End", "C++ STL Error"));
+  PastEndBugType->setSuppressOnSink(true);
----------------
How about: "C++ STL Error" -> "Misuse of STL APIs"


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:386
+// FIXME: Evaluation of these STL calls should be moved to StdCLibraryFunctions
+//       checker (see patch D20811) or another similar checker for C++ STL
+//       functions (e.g. StdCXXLibraryFunctions or StdCppLibraryFunctions).
----------------
Please, quote svn revision number instead of phabricator number.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:395
+  ASTContext &Ctx = C.getASTContext();
+  if (!II_std)
+    II_std = &Ctx.Idents.get("std");
----------------
You could simplify the code a bit by moving all these identifier lookups into a subrutine and/or just have a single statement guard checking f they have been initialized.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:445
+
+void IteratorPastEndChecker::handleComparison(CheckerContext &C,
+                                              const SVal &RetVal,
----------------
I agree with Artem that the future readers and maintainers of this code would greatly benefit if there were higher level comments explaining how this checker works. For example, here, we are saving the information about the comparison because iterators are value types...


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:721
+
+static bool inTopLevelNamespace(const Decl *D, IdentifierInfo *II) {
+  const auto *ND = dyn_cast<NamespaceDecl>(D->getDeclContext());
----------------
Would isInStdNamespace() from BugReporterVisitor.cpp be useful here? It would be fine to add this API to the CheckerContext or some other place accessible from here and the BugReporter.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:732
+
+static const RegionOrSymbol getRegionOrSymbol(const SVal &Val) {
+  if (const auto Reg = Val.getAsRegion()) {
----------------
This could be useful for other checkers as well. Maybe refactor this out as part of a subsequent commit?


================
Comment at: test/Analysis/Inputs/system-header-simulator-for-iterators.h:62
+                          ForwardIterator2 first2, ForwardIterator2 last2);
+}
----------------
baloghadamsoftware wrote:
> Maybe we should merge this file with the system-header-simulator-cxx.h? It already contains a vector type but no iterators.
Yes, we the headers are supposed to be reusable for different checkers!


================
Comment at: test/Analysis/iterator-past-end.cpp:73
+  auto first = std::find(vec.begin(), vec.end(), e);
+  *first; // expected-warning{{Iterator accessed past its end}}
+}
----------------
The error message is not very good for the find API cases. There is only a possibility of access past end. Also its much better to be explicit about what went wrong here - the user forgot to check the return value of find. We could say something like "The value returned from 'find' needs to be checked before it's accessed". 

We'd  need to implement a custom BugReporterVisitor that detects if the iterator is a return value from some method that needs checking. This can be & should be a separate patch.
 


https://reviews.llvm.org/D25660