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