njames93 created this revision.
njames93 added reviewers: aaron.ballman, steveire.
Herald added subscribers: usaxena95, kadircet, arphaman, kbarton, xazax.hun,
nemanjai.
njames93 updated this revision to Diff 325234.
njames93 added a comment.
njames93 updated this revision to Diff 325238.
njames93 updated this revision to Diff 325239.
njames93 published this revision for review.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added a project: clang.
Fix test
njames93 added a comment.
Disable formatting causing test to fail and document it.
njames93 added a comment.
Remove unintended change.
Adds a flag to ClangTidyContext that is used to indicate to checks that fixes
will only be applied one at a time.
This is to indicate to checks that each fix emitted should not depend on any
other fixes emitted across the translation unit.
I've currently implemented the IncludeInserter and PreferMemberInitializerCheck
to use these support these modes.
Reasoning behind this is use cases like clangd its only possible to apply one
fix at a time.
For include inserter checks, the include is only added once for the first
diagnostic that requires it, this will result in subsequent fixes not having
the included needed.
A similar issue is seen in the PreferMemberInitializerCheck where the `:` will
only be added for the first member that needs fixing.
Fixes emitted in SingleFixMode will likely result in malformed code if they are
applied all together, conversely fixes currently emitted may result in
malformed code if they are applied one at a time.
For this reason invoking clang-tidy from the binary will always with
SingleFixMode disabled, However using it as a library its possible to select
the mode you wish to use, clangd always selects SingleFixMode.
This is an example of the current behaviour failing
struct Foo {
int A, B;
Foo(int D, int E) {
A = D;
B = E; // Fix Here
}
};
Incorrectly transformed to:
struct Foo {
int A, B;
Foo(int D, int E), B(E) {
A = D;
// Fix Here
}
};
In SingleFixMode, it gets transformed to:
struct Foo {
int A, B;
Foo(int D, int E) : B(E) {
A = D;
// Fix Here
}
};
Repository:
rG LLVM Github Monorepo
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/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/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,7 +74,12 @@
return llvm::None;
// We assume the same Header will never be included both angled and not
// angled.
- if (!InsertedHeaders[FileID].insert(Header).second)
+ // In SingleFixMode inserting a header once in a translation unit should not
+ // prevent us from inserting it again.
+ if (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
@@ -181,7 +181,8 @@
<< Field;
bool AddComma = false;
- if (!Ctor->getNumCtorInitializers() && FirstToCtorInits) {
+ if (!Ctor->getNumCtorInitializers() &&
+ (FirstToCtorInits || isSingleFixMode())) {
SourceLocation BodyPos = Ctor->getBody()->getBeginLoc();
SourceLocation NextPos = Ctor->getBeginLoc();
do {
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 ran 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
