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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
