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

Tue Jul 28 05:29:21 PDT 2020

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

Let me double down -- just because we let some select people know about an option, I still don't think it should be user facing. I'm strongly against the philosophy that a core-modifying decision, such as configuring a state split should be presented on the default interface. Having "smart users" isn't an excuse to that either -- the last thing I'd like to see broadcasted is that you need to be a genius to use or configure a tool that is suppose to serve you, not the other way around.

================
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>
----------------
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.

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

https://reviews.llvm.org/D77150