[PATCH] D12761: MPI-Checker patch for Clang Static Analyzer

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

In http://reviews.llvm.org/D12761#304823, @Alexander_Droste wrote:

> Ah ok, I wasn't aware that clang-tidy is not restricted to checks which verify stylistic issues.
>  What makes it more convenient to integrate the checks in clang-tidy?  Is it how the AST-Matcher
>  functionality can be accessed?


Yes, less boilerplate code needed to work with AST matchers, powerful clang diagnostic infrastructure is used to report errors, also the add_new_check.py script ;)

> > I'm not an expert in the static analyzer code, so I'm not the one to review even AST-based checks there.

> 

> 

> The AST-based checks basically use zero functionality from the static analyzer codebase

>  except the entry callback and the functionality to generate bug reports.


Well, that's one of the ways to say that I have little experience and interest in reviewing patches to the static analyzer ;)

> > There's nothing that prevents adding more error-detecting checks to clang-tidy.

> 

> 

> Wrt. to the checks being part of this patch I have some concerns:

> 

> The first problem I see is that some functionlity is shared between the AST-based 

>  and path-sensitive checks. But I think this should be restricted to the `MPIFunctionClassifier` class.


If this class stays in the static analyzer, the clang-tidy check code could probably just depend on the relevant library and use this class?

> Another question I have is how this would affect the usability. At the moment

>  all detected bugs (AST-based and path-sensitive) appear in a single report,

>  generated by `scan-build`. Is there still a way to get the results in a single report 

>  if the checks get moved to `clang-tidy`?


clang-tidy can run static analyzer checks. However, it only supports the plain text output format, which is sub-optimal for representing the results of the path-based checks. So the single report scenario is not easy to implement right now. Gábor Horváth proposed a solution for this a few months ago (PList output for clang-tidy), but a proper implementation of this seemed to require some changes in the clang's diagnostic subsystem.

> Would the implementation need to change to AST-Matcher based?


That's not a 100% necessary step (you can always add a matcher for translationUnitDecl() and then traverse the AST using a RecursiveASTVisitor, for example), but this usually results in a more compact and easy to understand code.

> The point about the current implementation is that the AST-based checks

>  are also already backed by a range of tests defined in 

>  `tools/clang/test/Analysis/MPIChecker.cpp`.


Clang-tidy checks should also be backed by tests ;)


http://reviews.llvm.org/D12761