njames93 updated this revision to Diff 325608.
njames93 marked 2 inline comments as done.
njames93 added a comment.
Add test cases to clangd showing the improved behaviour of fix-its.
Rebased.
Update some comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97121/new/
https://reviews.llvm.org/D97121
Files:
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/abseil/StringFindStartswithCheck.cpp
clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
clang-tools-extra/clang-tidy/modernize/ReplaceAutoPtrCheck.cpp
clang-tools-extra/clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
clang-tools-extra/clang-tidy/performance/MoveConstructorInitCheck.cpp
clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp
clang-tools-extra/clang-tidy/utils/IncludeInserter.h
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp
===================================================================
--- clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp
@@ -30,8 +30,9 @@
public:
IncludeInserterCheckBase(StringRef CheckName, ClangTidyContext *Context,
utils::IncludeSorter::IncludeStyle Style =
- utils::IncludeSorter::IS_Google)
- : ClangTidyCheck(CheckName, Context), Inserter(Style) {}
+ utils::IncludeSorter::IS_Google,
+ bool SingleFixMode = false)
+ : ClangTidyCheck(CheckName, Context), Inserter(Style, SingleFixMode) {}
void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override {
@@ -85,6 +86,18 @@
}
};
+class MultipleHeaderSingleInserterCheck : public IncludeInserterCheckBase {
+public:
+ MultipleHeaderSingleInserterCheck(StringRef CheckName,
+ ClangTidyContext *Context)
+ : IncludeInserterCheckBase(CheckName, Context,
+ utils::IncludeSorter::IS_Google, true) {}
+
+ std::vector<StringRef> headersToInclude() const override {
+ return {"path/to/header.h", "path/to/header2.h", "path/to/header.h"};
+ }
+};
+
class CSystemIncludeInserterCheck : public IncludeInserterCheckBase {
public:
CSystemIncludeInserterCheck(StringRef CheckName, ClangTidyContext *Context)
@@ -246,6 +259,41 @@
PreCode, "clang_tidy/tests/insert_includes_test_input2.cc"));
}
+TEST(IncludeInserterTest, InsertMultipleIncludesNoDeduplicate) {
+ const char *PreCode = R"(
+#include "clang_tidy/tests/insert_includes_test_header.h"
+
+#include <list>
+#include <map>
+
+#include "path/to/a/header.h"
+
+void foo() {
+ int a = 0;
+})";
+ // FIXME ClangFormat bug - https://bugs.llvm.org/show_bug.cgi?id=49298
+ // clang-format off
+ const char *PostCode = R"(
+#include "clang_tidy/tests/insert_includes_test_header.h"
+
+#include <list>
+#include <map>
+
+#include "path/to/a/header.h"
+#include "path/to/header.h"
+#include "path/to/header2.h"
+#include "path/to/header.h"
+
+void foo() {
+ int a = 0;
+})";
+ // clang-format on
+
+ EXPECT_EQ(PostCode,
+ runCheckOnCode<MultipleHeaderSingleInserterCheck>(
+ PreCode, "clang_tidy/tests/insert_includes_test_input2.cc"));
+}
+
TEST(IncludeInserterTest, InsertBeforeFirstNonSystemInclude) {
const char *PreCode = R"(
#include "clang_tidy/tests/insert_includes_test_header.h"
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -517,6 +517,44 @@
ElementsAre(Diag(Main.range(), "do not use 'else' after 'return'")));
}
+TEST(DiagnosticTest, ClangTidySingleFixMode) {
+ Annotations Main(R"cpp(
+ struct Foo{
+ int A, B;
+ Foo()$Fix[[]] {
+ $A[[A = 1;]]
+ $B[[B = 1;]]
+ }
+ };
+ )cpp");
+ TestTU TU = TestTU::withCode(Main.code());
+ TU.ClangTidyProvider =
+ addTidyChecks("cppcoreguidelines-prefer-member-initializer");
+ clangd::Fix ExpectedAFix;
+ ExpectedAFix.Message =
+ "'A' should be initialized in a member initializer of the constructor";
+ ExpectedAFix.Edits.push_back(TextEdit{Main.range("Fix"), " : A(1)"});
+ ExpectedAFix.Edits.push_back(TextEdit{Main.range("A"), ""});
+
+ // When invoking clang-tidy normally, this code would produce `, B(1)` as the
+ // fix the `B` member, as it would think its already included the ` : ` from
+ // the previous `A` fix.
+ clangd::Fix ExpectedBFix;
+ ExpectedBFix.Message =
+ "'B' should be initialized in a member initializer of the constructor";
+ ExpectedBFix.Edits.push_back(TextEdit{Main.range("Fix"), " : B(1)"});
+ ExpectedBFix.Edits.push_back(TextEdit{Main.range("B"), ""});
+ EXPECT_THAT(
+ TU.build().getDiagnostics(),
+ UnorderedElementsAre(
+ AllOf(Diag(Main.range("A"), "'A' should be initialized in a member "
+ "initializer of the constructor"),
+ WithFix(EqualToFix(ExpectedAFix))),
+ AllOf(Diag(Main.range("B"), "'B' should be initialized in a member "
+ "initializer of the constructor"),
+ WithFix(EqualToFix(ExpectedBFix)))));
+}
+
TEST(DiagnosticsTest, Preprocessor) {
// This looks like a preamble, but there's an #else in the middle!
// Check that:
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -306,6 +306,7 @@
CTContext->setDiagnosticsEngine(&Clang->getDiagnostics());
CTContext->setASTContext(&Clang->getASTContext());
CTContext->setCurrentFile(Filename);
+ CTContext->setSingleFixMode();
CTChecks = CTFactories.createChecks(CTContext.getPointer());
llvm::erase_if(CTChecks, [&](const auto &Check) {
return !Check->isLanguageVersionSupported(CTContext->getLangOpts());
Index: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
@@ -30,8 +30,8 @@
TransformerClangTidyCheck::TransformerClangTidyCheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
- Inserter(
- Options.getLocalOrGlobal("IncludeStyle", IncludeSorter::IS_LLVM)) {}
+ Inserter(Options.getLocalOrGlobal("IncludeStyle", IncludeSorter::IS_LLVM),
+ isSingleFixMode()) {}
// This constructor cannot dispatch to the simpler one (below), because, in
// order to get meaningful results from `getLangOpts` and `Options`, we need the
Index: clang-tools-extra/clang-tidy/utils/IncludeInserter.h
===================================================================
--- clang-tools-extra/clang-tidy/utils/IncludeInserter.h
+++ clang-tools-extra/clang-tidy/utils/IncludeInserter.h
@@ -59,7 +59,8 @@
/// using \code
/// Options.getLocalOrGlobal("IncludeStyle", <DefaultStyle>)
/// \endcode
- explicit IncludeInserter(IncludeSorter::IncludeStyle Style);
+ explicit IncludeInserter(IncludeSorter::IncludeStyle Style,
+ bool SingleFixMode);
/// Registers this with the Preprocessor \p PP, must be called before this
/// class is used.
@@ -93,6 +94,7 @@
llvm::DenseMap<FileID, llvm::StringSet<>> InsertedHeaders;
const SourceManager *SourceMgr{nullptr};
const IncludeSorter::IncludeStyle Style;
+ const bool SingleFixMode;
friend class IncludeInserterCallback;
};
Index: clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp
+++ clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp
@@ -36,8 +36,9 @@
IncludeInserter *Inserter;
};
-IncludeInserter::IncludeInserter(IncludeSorter::IncludeStyle Style)
- : Style(Style) {}
+IncludeInserter::IncludeInserter(IncludeSorter::IncludeStyle Style,
+ bool SingleFixMode)
+ : Style(Style), SingleFixMode(SingleFixMode) {}
void IncludeInserter::registerPreprocessor(Preprocessor *PP) {
assert(PP && "PP shouldn't be null");
@@ -73,9 +74,14 @@
return llvm::None;
// We assume the same Header will never be included both angled and not
// angled.
- if (!InsertedHeaders[FileID].insert(Header).second)
- return llvm::None;
-
+ if (SingleFixMode) {
+ // Don't update the inserted set in SingleFixMode.
+ if (InsertedHeaders[FileID].contains(Header))
+ return llvm::None;
+ } else {
+ if (!InsertedHeaders[FileID].insert(Header).second)
+ return llvm::None;
+ }
return getOrCreate(FileID).CreateIncludeInsertion(Header, IsAngled);
}
Index: clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -69,7 +69,8 @@
StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
- utils::IncludeSorter::IS_LLVM)),
+ utils::IncludeSorter::IS_LLVM),
+ isSingleFixMode()),
AllowedTypes(
utils::options::parseStringList(Options.get("AllowedTypes", ""))) {}
Index: clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp
@@ -32,8 +32,8 @@
StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
- utils::IncludeSorter::IS_LLVM)) {
-}
+ utils::IncludeSorter::IS_LLVM),
+ isSingleFixMode()) {}
void TypePromotionInMathFnCheck::registerPPCallbacks(
const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
Index: clang-tools-extra/clang-tidy/performance/MoveConstructorInitCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/MoveConstructorInitCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/MoveConstructorInitCheck.cpp
@@ -24,7 +24,8 @@
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
- utils::IncludeSorter::IS_LLVM)) {}
+ utils::IncludeSorter::IS_LLVM),
+ isSingleFixMode()) {}
void MoveConstructorInitCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
Index: clang-tools-extra/clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
@@ -24,8 +24,8 @@
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
- utils::IncludeSorter::IS_LLVM)) {
-}
+ utils::IncludeSorter::IS_LLVM),
+ isSingleFixMode()) {}
void ReplaceRandomShuffleCheck::registerMatchers(MatchFinder *Finder) {
const auto Begin = hasArgument(0, expr());
Index: clang-tools-extra/clang-tidy/modernize/ReplaceAutoPtrCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/ReplaceAutoPtrCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/ReplaceAutoPtrCheck.cpp
@@ -41,7 +41,8 @@
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
- utils::IncludeSorter::IS_LLVM)) {}
+ utils::IncludeSorter::IS_LLVM),
+ isSingleFixMode()) {}
void ReplaceAutoPtrCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "IncludeStyle", Inserter.getStyle());
Index: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
@@ -121,7 +121,8 @@
PassByValueCheck::PassByValueCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
- utils::IncludeSorter::IS_LLVM)),
+ utils::IncludeSorter::IS_LLVM),
+ isSingleFixMode()),
ValuesOnly(Options.get("ValuesOnly", false)) {}
void PassByValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Index: clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -44,7 +44,8 @@
StringRef MakeSmartPtrFunctionName)
: ClangTidyCheck(Name, Context),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
- utils::IncludeSorter::IS_LLVM)),
+ utils::IncludeSorter::IS_LLVM),
+ isSingleFixMode()),
MakeSmartPtrFunctionHeader(
Options.get("MakeSmartPtrFunctionHeader", "<memory>")),
MakeSmartPtrFunctionName(
Index: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -486,7 +486,8 @@
MinConfidence(Options.get("MinConfidence", Confidence::CL_Reasonable)),
NamingStyle(Options.get("NamingStyle", VariableNamer::NS_CamelCase)),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
- utils::IncludeSorter::IS_LLVM)),
+ utils::IncludeSorter::IS_LLVM),
+ isSingleFixMode()),
UseCxx20IfAvailable(Options.get("UseCxx20ReverseRanges", true)),
ReverseFunction(Options.get("MakeReverseRangeFunction", "")),
ReverseHeader(Options.get("MakeReverseRangeHeader", "")) {
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
@@ -22,7 +22,8 @@
StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context), GslHeader(Options.get("GslHeader", "")),
Inserter(Options.getLocalOrGlobal("IncludeStyle",
- utils::IncludeSorter::IS_LLVM)) {}
+ utils::IncludeSorter::IS_LLVM),
+ isSingleFixMode()) {}
void ProBoundsConstantArrayIndexCheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/PreferMemberInitializerCheck.cpp
@@ -284,7 +284,7 @@
Diag << FixItHint::CreateInsertion(InsertPos, Insertion,
FirstToCtorInits)
<< FixItHint::CreateRemoval(StmtRange);
- FirstToCtorInits = false;
+ FirstToCtorInits = isSingleFixMode();
}
}
}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp
@@ -27,7 +27,8 @@
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
- utils::IncludeSorter::IS_LLVM)),
+ utils::IncludeSorter::IS_LLVM),
+ isSingleFixMode()),
MathHeader(Options.get("MathHeader", "<math.h>")) {}
void InitVariablesCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Index: clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
+++ clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
@@ -27,7 +27,8 @@
StringLikeClasses(utils::options::parseStringList(
Options.get("StringLikeClasses", "::std::basic_string"))),
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
- utils::IncludeSorter::IS_LLVM)),
+ utils::IncludeSorter::IS_LLVM),
+ isSingleFixMode()),
AbseilStringsMatchHeader(
Options.get("AbseilStringsMatchHeader", "absl/strings/match.h")) {}
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -175,6 +175,10 @@
return AllowEnablingAnalyzerAlphaCheckers;
}
+ void setSingleFixMode(bool Value = true) { SingleFixMode = Value; }
+
+ bool isSingleFixMode() const { return SingleFixMode; }
+
using DiagLevelAndFormatString = std::pair<DiagnosticIDs::Level, std::string>;
DiagLevelAndFormatString getDiagLevelAndFormatString(unsigned DiagnosticID,
SourceLocation Loc) {
@@ -210,6 +214,7 @@
std::string ProfilePrefix;
bool AllowEnablingAnalyzerAlphaCheckers;
+ bool SingleFixMode;
};
/// Check whether a given diagnostic should be suppressed due to the presence
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -157,7 +157,8 @@
bool AllowEnablingAnalyzerAlphaCheckers)
: DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
Profile(false),
- AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers) {
+ AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers),
+ SingleFixMode(false) {
// Before the first translation unit we can get errors related to command-line
// parsing, use empty string for the file name in this case.
setCurrentFile("");
Index: clang-tools-extra/clang-tidy/ClangTidyCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyCheck.h
+++ clang-tools-extra/clang-tidy/ClangTidyCheck.h
@@ -511,6 +511,9 @@
StringRef getCurrentMainFile() const { return Context->getCurrentFile(); }
/// Returns the language options from the context.
const LangOptions &getLangOpts() const { return Context->getLangOpts(); }
+ /// Returns true when the check is run in a use case when only 1 fix will be
+ /// applied at a time.
+ bool isSingleFixMode() const { return Context->isSingleFixMode(); }
};
/// Read a named option from the ``Context`` and parse it as a bool.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
