[PATCH] D29151: [clang-tidy] Add misc-invalidated-iterators check.

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek added a comment.

In https://reviews.llvm.org/D29151#668197, @zaks.anna wrote:

> > I tried to come up with some advice on where checks should go in http://clang.llvm.org/extra/clang-tidy/#choosing-the-right-place-for-your-check
>
> The guidelines stated on the clang-tidy page seem reasonable to me.
>
> > For example, IMO, AST-based analyses make more sense in clang-tidy for a few reasons:
> > 
> > They usually are easier expressible in terms of AST matchers, and clang-tidy allows to use AST matchers with less boilerplate.
> >  Clang Static Analyzer is linked into clang, where AST matchers are undesired, since they tend to blow the size of binary significantly.
> >  It's more consistent to keep all similar analyses together, it simplifies search for already implemented similar functionality and code reviews.
>
> I agree that there is a gray area specifically in the AST-matching-based bug finding, which unfortunately leads to duplication of effort. However, we currently cannot ban/move all AST-based checks out of the analyzer. The main problem I see with restricting the AST-based analysis to clang-tidy is impact on the end users. While clang-tidy does export the clang static analyzer results, the reverse does not hold. There are many end users who do not use clang-tidy but do use the clang static analyzer (either directly or through scan-build). Until that is the case, I do not think we should disallow AST based checks from the analyzer, especially if they come from contributors who are interested in contributing to this tool. Note that the format in which the results are presented from these tools is not uniform, which makes transition more complicated.
>
> The AST matchers can be used from the analyzer and if the code size becomes a problem, we could consider moving the analyzer out of the clang codebase.
>
> Generally, I am not a big fan of the split between the checks based on the technology used to implement them since it does not lead to a great user interface - the end users do not think in terms of path-sensitive/flow-sensitive/AST-matching, they just want to enable specific functionality such as find bugs or ensure they follow coding guidelines. This is why I like the existing guidelines with their focus on what clang-tidy is from user perspective:


Agree with it.

>> clang-tidy check is a good choice for linter-style checks, checks that are related to a certain coding style, checks that address code readability, etc.
> 
> Another thing that could be worth adding here is that clang-tidy finds bugs that tend to be easy to fix and, in most cases, provide the fixits. (I believe, this is one of the major strengths of the clang-tidy tool!)
> 
>> Flow-sensitive analyses (that don't need any path-sensitivity) seem to be equally suitable for SA and clang-tidy (unless I'm missing something), so I don't feel strongly as to where they should go.
> 
> As far as I know, currently, all flow-sensitive analysis are in the Analysis library in clang. These are analysis for compiler warnings and analysis used by the static analyzer. I believe this is where future analysis should go as well, especially, for analysis that could have multiple clients. If clang code size impact turns out to be a serious problem, we could also have that library provide APIs that could be used from other tools/checks. (Note, the analysis could be implemented in the library, but the users of the analysis (checks) can be elsewhere.)
> 
> Regarding the points about "heuristics" and "flexibility", the analyzer is full of heuristics and fuzzy API matching. These techniques are very common in static analysis in general as we are trying to solve undecidable problems and heuristics is the only way to go.

I guess the main problem is that you can't use clang-tidy checks from scan build, where there are some checks like use-after-move, and bunch of misc checks, that would totally fit into what user want from scan build.


Repository:
  rL LLVM

https://reviews.llvm.org/D29151