Szelethus added a comment. I don't see obvious red flags here, @NoQ?
In D62525#1523026 <https://reviews.llvm.org/D62525#1523026>, @baloghadamsoftware wrote: > In D62525#1519868 <https://reviews.llvm.org/D62525#1519868>, @NoQ wrote: > > > In D62525#1519475 <https://reviews.llvm.org/D62525#1519475>, > > @baloghadamsoftware wrote: > > > > > Before someone asks: `NoteTag`s are not applicable here since > > > invalidation and reaching one end of the range happens in many different > > > places. This is also true for container emptiness. > > > > > > Mm, what's wrong with many different places? If you're worried about code > > duplication, just put tag construction into a function (?) > > > It is not about code duplication. Of course, that can be handled by a > function. > > The problem is that the transition is added in the top level function but > iterator position adjustments happen in the lower level ones. Data flow is > one-way, top-down. For example, an insertion happens into a vector that > invalidates all the iterators at and after the position where it is inserted. > In this case `handleInsert()` calls a function that iterates all the active > iterator positions and calls other one that invalidates the ones affected by > the insertion. To propagate back the list of iterators invalidated would be > too complex. Increments and decrements are even worse. Thus here the correct > way seems to find the relevant transitions in a visitor instead of trying to > figure out what note tags to add at each transition. Mhm, I personally find this reasoning sufficient. ================ Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:281-282 + + // `FoundChange` becomes true when we find the statement the results in the + // current state of the iterator. + // `FoundEmptyness` becomes true when we find the block edge assuming ---------------- baloghadamsoftware wrote: > NoQ wrote: > > I don't think we should stop here. I think it's worth it to highlight *all* > > increments and decrements of the interesting iterator, so that it was clear > > how come that it has the given value. > > > > Additionally, because iterators are copied around, it makes sense to > > "recursively" apply the visitor to the original object when the iterator is > > obtained as a copy (similarly to how `trackExpressionValue` works). > This can be done but an iterator can reach the past-the-end or the first > position by changing the container's size as well, not only by incrementing > or decrementing the iterator itself. Maybe in a followup patch, but I'd love to see a `trackIterator` function! ================ Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1277-1292 + State = setContainerData(State, ContReg, CData->newEnd(OldEndSym)); } else { State = setContainerData(State, ContReg, - ContainerData::fromEnd(NewEndSym)); + ContainerData::fromEnd(OldEndSym)); } + // Then generate and assign a new "end" symbol for the old container. + auto NewEndSym = ---------------- baloghadamsoftware wrote: > Szelethus wrote: > > I'm confused, are these changes related to this patch? It doesn't seem to > > be. > Yes, they are. Since we want to track the iterator positions upwards along > the bugpath as far a possible I had to reverse one of my decisions. When I > first decided it was really like a coin thrown up, but now it turned out I > took the wrong choice considering the visitor. The change is that upon move > assignment of a container the iterators are transferred to the new container, > except the past-the-end iterator. However we also have iterator positions > relative to the past-the-end iterator (thus using the same symbol) which must > be transferred. So I had to separate them from the past-the-end positions by > generating a new end symbol. I originally used this new symbol for the > positions to be transferred and kept the old symbol for the past-the-end > positions. However this makes tracking of the non past-the-end positions very > difficult so I reversed it and now I transfer the old symbol and use the new > for the past-the-end positions. Sometimes I just need a reality check to realize how absurdly difficult the problem is that you're solving... Wow. I mentioned this elsewhere, but do we not need a debug checker to validate that we argue about all of these correctly? ================ Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1495-1496 // For deque-like containers invalidate all iterator positions. For // vector-like containers invalidate iterator positions after the insertion. const auto *Cont = Pos->getContainer(); ---------------- >>! In D62525#1523026, @baloghadamsoftware wrote: > For example, an insertion happens into a vector that invalidates all the > iterators at and after the position where it is inserted. Is this actually correct? https://en.cppreference.com/w/cpp/container/deque > std::deque (double-ended queue) is an indexed sequence container that allows > fast insertion and deletion at both its beginning and its end. In addition, > insertion and deletion at either end of a deque never invalidates pointers or > references to the rest of the elements. https://en.cppreference.com/w/cpp/container/vector > `insert`, `emplace`, `resize`: If the vector changed capacity, all of the > iterators are invalidated. If not, only those after the insertion point. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62525/new/ https://reviews.llvm.org/D62525 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
