[PATCH] D77150: [Analyzer] New Option for ContainerModeling: AggressiveEraseModeling

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D77150#2149870 <https://reviews.llvm.org/D77150#2149870>, @dkrupp wrote:

> Since the analyzer cannot cannot model the size of the containers just yet (or precisely enough), what we are saying with this checker is to "always check the return value of the erase() function before use (for increment/decrement etc.) whether it is not past-end" .
>
> Adam, are you sure that the user would like to enforce such a generic coding rule? Depending on the actual code analyzed, this would require this clumsy check at potentially too many places.

While I agree that realising in the analyser that the user code at hand is meant to express that the iterator stepping will not go out of bounds, you have to keep in mind, that going out of bounds with an iterator is still UB. In case of a `for` loop, it's //always// a dangerous construct to call `erase()` inside, unless you, indeed, can explicitly ensure that you won't go out of bounds.

Side note: I am surprised there isn't a Tidy check in `bugprone-` that simply says //"If you are using `erase()`, use a `while` loop and not a `for` loop..."//

I'm not terribly up-to-date with @baloghadamsoftware's patches (nor the innards of the analyser), but as a user who'd get this warning, I believe one crucial information missing is something like //"note: assuming `erase()` removed the the last element, setting `it` to the past-the-end iterator"//

In D77150#2149870 <https://reviews.llvm.org/D77150#2149870>, @dkrupp wrote:

> Oops, I did not mean "request changes". This is just meant to be an additional comment in this discussion.

You can do the //Resign as Reviewer// action to remove your `X` from the patch. 🙂



================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:620-624
+                  "AggressiveEraseModeling",
+                  "Enables exploration of the past-the-end branch for the "
+                  "return value of the erase() method of containers.",
+                  "false",
+                  Released>
----------------
Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > Hmm. The description isn't really user-friendly, imo, but the option doesn't sound too user-facing either. I don't think this is an option to be tinkered with. I think we should make this hidden.
> > > 
> > > Also, even for developers, I find this a bitconfusing at first. Do we not enable that by default? Why not? What do we have to gain/lose?
> > What is the rule that the user needs to follow in order to avoid the warning? "Always check the return value of `erase()` before use"? If so, a good description would be along the lines of "Warn when the return value of `erase()` is not checked before use; compare that value to end() in order to suppress the warning" (or something more specific).
> Please mark this hidden.
Is marking something hidden still allow for modifying the value from the command-line, or via CodeChecker?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:708-709
+      // end symbol from the container data because the  past-the-end iterator
+      // was also invalidated. The next call to member function `end()` would
+      // create a new one, but we need it now so we create it now.
+      if (!EndSym) {
----------------
Is this creation prevented?


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

https://reviews.llvm.org/D77150