Following on from D117129, This changes fundamentally how IncludeInserter works.
Now each check that wants to create includes just calls registerIncludeInserter in their constructor.
This in turn enables a shared IncludeInserter in the ClangTidyContext.
Going this route means only 1 instance of IncludeInserter is needed and prevents different checks conflicting when they both try to create the same include.
Only using 1 instance also means there is only 1 possible IncludeStyle which is now sourced from global check options solely, rather that getting a local option for each check that wants to use it. This is a partially breaking change so warnings have been added for any old configs that try to use local options to set IncludeStyle.
Details
- Reviewers
aaron.ballman LegalizeAdulthood
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/clang-tidy/ClangTidy.cpp | ||
---|---|---|
434 | Why not just Context.registerIncludeInserter(PP) and follow the Law of Demeter? Let Context do the hasIncludeInserter() check itself. | |
clang-tools-extra/clang-tidy/ClangTidyCheck.cpp | ||
220–238 | Law of Demeter. Move this to ClangTidyContext and let it handle the details. | |
clang-tools-extra/clang-tidy/ClangTidyCheck.h | ||
420 | diag has visibility public. Any reason why these are visibility protected? | |
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp | ||
219 | is this-> necessary here? | |
clang-tools-extra/clang-tidy/IncludeInserter.h | ||
20 | What's the guidance on what belongs in clang::tidy | |
clang-tools-extra/clang-tidy/IncludeSorter.h | ||
14 | Since you introduced ArrayRef<StringRef> as a data type, I was expecting "Include what you use" would seem to imply that we should be including:
| |
clang-tools-extra/clangd/ParsedAST.cpp | ||
432–433 | Law of Demeter | |
clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h | ||
71–72 | Law of Demeter | |
clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp | ||
259–289 | Am I correct in thinking that these are now redundant tests |
clang-tools-extra/clang-tidy/ClangTidyCheck.cpp | ||
---|---|---|
220–238 | Are you suggesting I create methods for createIncludeInsertion in the ClangTidyContext? | |
clang-tools-extra/clang-tidy/IncludeInserter.h | ||
20 | Well as the file moved out of utils, its no longer in the utils namespace. | |
clang-tools-extra/clang-tidy/IncludeSorter.h | ||
14 | IWYU should strictly be followed but in practice throughout clang/llvm it never really is. | |
clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp | ||
259–289 | This test is redundant as checks no longer look for a local IncludeStyle instead just reading the global IncludeStyle which is what the following test is checking for. |
clang-tools-extra/clang-tidy/ClangTidyCheck.cpp | ||
---|---|---|
220–238 | Yes, exactly. Delegate everything to the Context object, instead of | |
clang-tools-extra/clang-tidy/ClangTidyCheck.h | ||
128–131 | Add doxygen comments, please | |
clang-tools-extra/clang-tidy/IncludeInserter.h | ||
20 | Yes, if you move the file out of utils then it shouldn't be in the IOW, what is the guideline for what files belong under utils |
clang-tools-extra/clang-tidy/IncludeInserter.h | ||
---|---|---|
20 | Utils is for utilities that checks can make use of, but aren't required for the base clang-tidy infra to work. |
clang-tools-extra/clang-tidy/IncludeInserter.h | ||
---|---|---|
20 | OK yeah, that's a good explanation, thanks! |
If the only thing holding this up is my comments about following the Law of Demeter, I don't think that should block submitting this change.
I don't feel strongly about it enough to block submission and the style guide doesn't say anything about it.
Why not just Context.registerIncludeInserter(PP) and follow the Law of Demeter?
Let Context do the hasIncludeInserter() check itself.