njames93 created this revision.
Herald added subscribers: carlosgalvezp, usaxena95, kadircet, arphaman, kbarton, xazax.hun, mgorny, nemanjai.
Eugene.Zelenko added reviewers: aaron.ballman, LegalizeAdulthood.
Eugene.Zelenko added a project: clang-tools-extra.
njames93 updated this revision to Diff 400365.
njames93 added a comment.
njames93 marked 10 inline comments as done.
njames93 updated this revision to Diff 400472.
njames93 marked 3 inline comments as done.
njames93 published this revision for review.
Herald added a subscriber: cfe-commits.
Update some includes
njames93 added a comment.
Address comments
================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:489
+ if (Context.hasIncludeInserter()) {
+ auto Mapping = IncludeSorter::getStyleMapping();
+ auto Index = static_cast<unsigned>(Context.getIncludeInserter().getStyle());
----------------
Please do not use `auto`, because type is not spelled explicitly in statement.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:434
+ if (Context.hasIncludeInserter())
+ Context.getIncludeInserter().registerPreprocessor(PP);
+
----------------
Why not just `Context.registerIncludeInserter(PP)` and follow the Law of Demeter?
Let `Context` do the `hasIncludeInserter()` check itself.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:220-238
+llvm::Optional<FixItHint>
+ClangTidyCheck::createMainFileIncludeInsertion(StringRef Include) {
+ if (!Context->hasIncludeInserter()) {
+ // Only crash on debug builds
+ assert(false && "No IncludeInserter registered");
+ return llvm::None;
+ }
----------------
Law of Demeter. Move this to `ClangTidyContext` and let it handle the details.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:220-238
+llvm::Optional<FixItHint>
+ClangTidyCheck::createMainFileIncludeInsertion(StringRef Include) {
+ if (!Context->hasIncludeInserter()) {
+ // Only crash on debug builds
+ assert(false && "No IncludeInserter registered");
+ return llvm::None;
+ }
----------------
LegalizeAdulthood wrote:
> Law of Demeter. Move this to `ClangTidyContext` and let it handle the details.
Are you suggesting I create methods for createIncludeInsertion in the ClangTidyContext?
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:420
+
+ llvm::Optional<FixItHint> createMainFileIncludeInsertion(StringRef Include);
+ llvm::Optional<FixItHint> createIncludeInsertion(FileID File,
----------------
`diag` has visibility public. Any reason why these are visibility `protected`?
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:219
std::make_unique<CachedGlobList>(*getOptions().WarningsAsErrors);
+ this->Inserter.reset();
}
----------------
is `this->` necessary here?
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:569
+ StringRef Closest;
+ auto Mapping = IncludeSorter::getStyleMapping();
+ for (unsigned I = 0, E = Mapping.size(); I < E; ++I) {
----------------
Ditto.
================
Comment at: clang-tools-extra/clang-tidy/IncludeInserter.h:20
namespace tidy {
-namespace utils {
----------------
What's the guidance on what belongs in `clang::tidy`
and what belongs in `clang::tidy::utils`?
================
Comment at: clang-tools-extra/clang-tidy/IncludeInserter.h:20
namespace tidy {
-namespace utils {
----------------
LegalizeAdulthood wrote:
> What's the guidance on what belongs in `clang::tidy`
> and what belongs in `clang::tidy::utils`?
Well as the file moved out of utils, its no longer in the utils namespace.
================
Comment at: clang-tools-extra/clang-tidy/IncludeSorter.h:14
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/StringMap.h"
#include <string>
----------------
Since you introduced `ArrayRef<StringRef>` as a data type, I was expecting
the includes for those to be included instead of `StringMap`; are we really
using `StringMap` in this header?
"Include what you use" would seem to imply that we should be including:
- StringRef
- ArrayRef
- Optional
- FixItHint
================
Comment at: clang-tools-extra/clang-tidy/IncludeSorter.h:14
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/StringMap.h"
#include <string>
----------------
LegalizeAdulthood wrote:
> Since you introduced `ArrayRef<StringRef>` as a data type, I was expecting
> the includes for those to be included instead of `StringMap`; are we really
> using `StringMap` in this header?
>
> "Include what you use" would seem to imply that we should be including:
> - StringRef
> - ArrayRef
> - Optional
> - FixItHint
IWYU should strictly be followed but in practice throughout clang/llvm it never really is.
We are actually using StringMap in this header.
All I really did here was using clangd's include information fix-its to make it compile.
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:432-433
Preprocessor *PP = &Clang->getPreprocessor();
+ if (CTContext->hasIncludeInserter())
+ CTContext->getIncludeInserter().registerPreprocessor(PP);
for (const auto &Check : CTChecks) {
----------------
Law of Demeter
================
Comment at: clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h:71-72
+
+ if (Context.hasIncludeInserter())
+ Context.getIncludeInserter().registerPreprocessor(PP);
+
----------------
Law of Demeter
================
Comment at: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp:259-289
-TEST(TransformerClangTidyCheckTest, AddIncludeObeysSortStyleLocalOption) {
- std::string Input = R"cc(#include "input.h"
-int h(int x) { return 3; })cc";
-
- std::string TreatsAsLibraryHeader = R"cc(#include "input.h"
-
-#include "bar.h"
----------------
Am I correct in thinking that these are now redundant tests
because IncludeInserterTest covers these cases?
================
Comment at: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp:259-289
-TEST(TransformerClangTidyCheckTest, AddIncludeObeysSortStyleLocalOption) {
- std::string Input = R"cc(#include "input.h"
-int h(int x) { return 3; })cc";
-
- std::string TreatsAsLibraryHeader = R"cc(#include "input.h"
-
-#include "bar.h"
----------------
LegalizeAdulthood wrote:
> Am I correct in thinking that these are now redundant tests
> because IncludeInserterTest covers these cases?
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.
Following on from D117129 <https://reviews.llvm.org/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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D117409
Files:
clang-tools-extra/clang-tidy/CMakeLists.txt
clang-tools-extra/clang-tidy/ClangTidy.cpp
clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
clang-tools-extra/clang-tidy/ClangTidyCheck.h
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tools-extra/clang-tidy/IncludeInserter.cpp
clang-tools-extra/clang-tidy/IncludeInserter.h
clang-tools-extra/clang-tidy/IncludeSorter.cpp
clang-tools-extra/clang-tidy/IncludeSorter.h
clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.h
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.h
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.h
clang-tools-extra/clang-tidy/misc/UniqueptrResetReleaseCheck.cpp
clang-tools-extra/clang-tidy/misc/UniqueptrResetReleaseCheck.h
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.h
clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
clang-tools-extra/clang-tidy/modernize/PassByValueCheck.h
clang-tools-extra/clang-tidy/modernize/ReplaceAutoPtrCheck.cpp
clang-tools-extra/clang-tidy/modernize/ReplaceAutoPtrCheck.h
clang-tools-extra/clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
clang-tools-extra/clang-tidy/modernize/ReplaceRandomShuffleCheck.h
clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp
clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.h
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.h
clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h
clang-tools-extra/clang-tidy/utils/CMakeLists.txt
clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp
clang-tools-extra/clang-tidy/utils/IncludeInserter.h
clang-tools-extra/clang-tidy/utils/IncludeSorter.cpp
clang-tools-extra/clang-tidy/utils/IncludeSorter.h
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp
clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D117409.400472.patch
Type: text/x-patch
Size: 61509 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220117/febfd7bf/attachment-0001.bin>