This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] Iterator Checker - Part 5: Move Assignment of Containers
ClosedPublic

Authored by baloghadamsoftware on May 4 2017, 5:29 AM.

Details

Summary

If a container is moved by its move assignment operator, according to the standard all their iterators except the past-end iterators remain valid but refer to the new container. This patch introduces support for this case in the iterator checkers.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ added inline comments.Dec 14 2017, 3:59 PM
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
1619–1641

I do not immediately understand what is this useful for. At least tests don't look like they make use of these offset manipulations(?)

Without full understanding, i wonder: when we overwrite one container with another, why don't we just overwrite all symbols associated with it, instead of creating a mixture of old and new symbols?

Or maybe this is an accidental part of another patch, that has something to do with resizes?

Rebased to current part 4. New tests added. Comments added.

baloghadamsoftware edited reviewers, added: dcoughlin; removed: zaks.anna.Jan 17 2018, 7:42 AM

One test failed after rebased to the current master branch. Depending on the internal implementation of the iterator and the move operation of the container it could happen that after moving a container the begin() or end() of the target of the move returns an already existing iterator position to the source of the move which is wrong. Therefore we must first check for begin() and end() and only then check for the existence of the iterator position of the return value of a function returning an iterator.

george.karpenkov resigned from this revision.May 30 2018, 4:51 PM
whisperity added inline comments.Jul 5 2018, 5:07 AM
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
1038

( is not closed

In D32859##inline-360206, @NoQ wrote:

I do not immediately understand what is this useful for. At least tests don't look like they make use of these offset manipulations(?)

Without full understanding, i wonder: when we overwrite one container with another, why don't we just overwrite all symbols associated with it, instead of creating a mixture of old and new symbols?

Or maybe this is an accidental part of another patch, that has something to do with resizes?

I do not see which lines exactly you commented but I suppose it is about not reassigning all iterator positions to the new container upon moving. According to [[ C++ Reference | en.cppreference.com/w/cpp/container/vector/operator%3D ]] the past-end iterators are not moved to the new container.

NoQ accepted this revision.Aug 14 2018, 3:22 PM

I think this patch is in good shape.

I do not see which lines exactly you commented

This was about the replaceSymbol() sub. You can traverse history using the History tab, or if there's still a greyed out comment you can push the |<< button on it to jump to the respective historical diff.

In any case, i think in current form replaceSymbol() is much easier to understand, but still deserves a short comment, maybe a small example, maybe a better name (the word "rebase" comes to mind).

This revision is now accepted and ready to land.Aug 14 2018, 3:22 PM

Comments added, functions renamed.

baloghadamsoftware marked an inline comment as done.Sep 3 2018, 8:46 AM
This revision was automatically updated to reflect the committed changes.