[PATCH] D98710: [clang-tidy] New feature --skip-headers, part 1, setTraversalScope

[Chih-Hung Hsieh via Phabricator via cfe-commits]

chh added a comment.

Sam,

The latest tested change contained one of your suggestions:

  using setTraversalScope for all consumers in the
  ClangTidyASTConsumer::HandleTranslationUnit

It is good to pass all existing tests,
although not a proof of correctness yet.

The failed libarcher.* tests have been there for days.
We can be sure that those are unrelated to this change.

We still have some different views of the purpose and requirements of
these new flags, skip-headers and show-all-warnings. That's why it is
important to describe them correctly in the summary.
We can try to include more use cases or applications now or later,
however, the following has been our requirement for a successful story
for the first Android deployment:

  --skip-headers should work correctly, even if not at optimal speed.
  
  Correctness means no more or fewer "displayed" warnings with or without
  this flag, although it could report fewer "suppressed" header file warnings.

For desired performance gain, experiment data showed that it is more than
enough to get savings from only MatchFinder-based checks.

We are less critical on "implementation" methods, code complexity, or efficiency.
Using set/getTraversalScope or not, cutting Decls in advance or on the fly,
cutting only top-level Decls or all Decls at any level, are all acceptable
alternatives if they produce the same "displayed" warnings as before.

Please also take a look of https://reviews.llvm.org/D98709,
which skips Decls at all levels on-the-fly.
That is a different implementation with 50% less changes in ClangTidy.cpp
than this one. The changes in other files are identical or tiny.
It is a little slower, but D98709 <https://reviews.llvm.org/D98709> is smaller and simpler to maintain,
and it limits the impact to only MatchFinder, not static analyzer.

Now the bad news is that when tested against the whole Android source, 
I found failed cases, which are missing clang-tidy warning messages
by both implementations.

The missed warnings were bugprone-forward-declaration-namespace.
There could be other misses undetected.
According to bugprone/ForwardDeclarationNamespaceCheck.cpp,
it's a MatchFinder-based check that can report warning on
Decl in the main file, but the check needs Decls in an
included file. For this kind of checks, skipping their
matchers for Decls in header files is wrong.
This is similar to what we found before that some checks
need the root TranslationUnit node in AST, so we have to
change set/getTraversalScope to keep the root node.

Now we realized that we cannot simply skip or cut out Decls
of headers files for ALL MatchFinder-based checks. Some checks
or some of their matchers need to see ALL Decls in ALL source files.

I will try to update this D98710 <https://reviews.llvm.org/D98710> and D98709 <https://reviews.llvm.org/D98709> with new test cases
to show the failed bugprone-forward-declaration-namespace checks.
Then, I will try some implementation changes to work also for
those tidy checks.  The implementation probably won't be as simple
as this one to cut all top-level header file Decls for all checks.


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

https://reviews.llvm.org/D98710