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

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

baloghadamsoftware added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:195
+    auto Param = State->getLValue(P, LCtx);
+    auto Arg = State->getSVal(CE->getArg(idx++), LCtx->getParent());
+    const auto *Pos = getIteratorPosition(State, Arg);
----------------
NoQ wrote:
> I think this trick needs more comments/explaining. It is very unusual. Are you trying to model effects of passing an iterator by value into a function? What part of these effects are not modeled magically by the core?
If I pass an iterator by value (the most usual case) I have to assign its position (in or out of range) to the formal parameter from the actual one.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:568
+  // We also should check for copy assignment operator, but for some reason
+  // it is not provided implicitly in Clang Static Analyzer
+  for (const auto *M : CRD->methods()) {
----------------
NoQ wrote:
> It's not analyzer's fault :) We're inspecting the AST here.
> 
> Anyway, does `CXXRecordDecl::needsImplicitCopyAssignment()` look useful?
No, it does not. I need to check whether the type is copiable, since that is a criteria for being an operator (copiable via constructor and assignment, deleteable, incrementable and dereferencable). It seems that while copy constructor and destructor is generated automatically, copy assignment not, at least not in this simple case. So I defaulted it to true, and I set it to false if I find a deleted or a non-public copy assignment.


================
Comment at: test/Analysis/iterator-past-end.cpp:3
+
+template <typename T, typename Ptr, typename Ref> struct __iterator {
+  typedef __iterator<T, T *, T &> iterator;
----------------
NoQ wrote:
> We should probably separate this into an #include-able header in `test/Analysis/Inputs/`.
> 
> Also, there's always a bit of concern that it wasn't copy-pasted from a standard library implementation with an incompatible license such as (L)GPL. Which often happens when you do your best to emulate the normal way of defining things as closely as possible.
I did it now, but first one of my tests failed. I fixed the bug, but it turned out that if I include these types and functions, no method or function is checked, just conjured symbols are generated. Should including not behave the same as copying the contents? This happened even if I removed the pragma.


https://reviews.llvm.org/D25660