[PATCH] D74615: [Analyzer] Add visitor to track iterator invalidation

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

baloghadamsoftware added a comment.

In D74615#1923710 <https://reviews.llvm.org/D74615#1923710>, @NoQ wrote:

> In D74615#1917289 <https://reviews.llvm.org/D74615#1917289>, @Szelethus wrote:
>
> > You may have explained it in the summary, and I didn't get it, but why isn't putting a `NoteTag` in `invalidateAllIteratorPositions` sufficient? We could state something like `All iterators associated with 'V' are invalidated`.
>
>
> +1, that's the intended approach. I suspect we don't even need to simplify the message.


That is planned (a third patch for Container Note Tags), but does not fully solve the problem. There could be multiple operations for the same container which invalidates //some// iterators. Each operation creates a not tag which explains the kind of iterators invalidated (e.g. all iterators after the iterator passed in the parameter). However that does not explain which operation invalidates the particular iterator which is accessed.

Generally, I do not think the note tags should completely replace custom visitors. Instead we should take the approach which is more convenient in each case. The basic rule I can think of is the following: if the checker //does// something with a value when adding a new transition then we should assign a note tag to that transition. However, if something //happens// to the value in a transition which is done by another checker (or the core engine) then we should use a visitor to find that transition.

In this case the container modeling shrinks and extends the container and invalidates groups of iterators. However, from the point of view of an iterator the invalidation happens in another checker (`ContainerModeling`). Therefore I still think that a visitor is needed to mark the point where that particular iterator was invalidated. This may result in double notes like `All iterators of ``V`` after ``i1`` are invalidated.` and then the next is `Iterator ``i0`` is invalidated.` but this is not disturbing. We already have such double notes: `Assuming the condition is ``true``.` then `Taking ``true``branch.`.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74615/new/

https://reviews.llvm.org/D74615