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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
