njames93 updated this revision to Diff 326779.
njames93 marked 6 inline comments as done.
njames93 added a comment.
Rebased and address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84924/new/
https://reviews.llvm.org/D84924
Files:
clang-tools-extra/clang-tidy/ClangTidy.cpp
clang-tools-extra/clang-tidy/ClangTidy.h
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/index.rst
clang-tools-extra/test/clang-tidy/checkers/misc-definitions-in-headers.hpp
clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls-cxx17.cpp
clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls.cpp
clang-tools-extra/test/clang-tidy/checkers/readability-inconsistent-declaration-parameter-name.cpp
clang-tools-extra/test/clang-tidy/infrastructure/alternative-fixes.cpp
clang-tools-extra/test/clang-tidy/infrastructure/clean-up-code.cpp
Index: clang-tools-extra/test/clang-tidy/infrastructure/clean-up-code.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/infrastructure/clean-up-code.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/clean-up-code.cpp
@@ -1,6 +1,6 @@
-// RUN: %check_clang_tidy %s misc-unused-using-decls %t
-// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- -format-style=none --
-// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- -format-style=llvm --
+// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- --fix-notes
+// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- --fix-notes -format-style=none --
+// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- --fix-notes -format-style=llvm --
namespace a { class A {}; }
namespace b {
using a::A;
Index: clang-tools-extra/test/clang-tidy/infrastructure/alternative-fixes.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/infrastructure/alternative-fixes.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/alternative-fixes.cpp
@@ -1,9 +1,10 @@
-// RUN: %check_clang_tidy %s "llvm-namespace-comment,clang-diagnostic-*" %t
+// RUN: %check_clang_tidy %s "llvm-namespace-comment,clang-diagnostic-*" %t -- --fix-notes
void foo(int a) {
if (a = 1) {
- // CHECK-NOTES: [[@LINE-1]]:9: warning: using the result of an assignment as a condition without parentheses [clang-diagnostic-parentheses]
- // CHECK-NOTES: [[@LINE-2]]:9: note: place parentheses around the assignment to silence this warning
- // CHECK-NOTES: [[@LINE-3]]:9: note: use '==' to turn this assignment into an equality comparison
- // CHECK-FIXES: if ((a = 1)) {
+ // CHECK-NOTES: [[@LINE-1]]:9: warning: using the result of an assignment as a condition without parentheses [clang-diagnostic-parentheses]
+ // CHECK-NOTES: [[@LINE-2]]:9: note: place parentheses around the assignment to silence this warning
+ // CHECK-NOTES: [[@LINE-3]]:9: note: use '==' to turn this assignment into an equality comparison
+ // As we have 2 conflicting fixes in notes, don't apply any fix.
+ // CHECK-FIXES: if (a = 1) {
}
}
Index: clang-tools-extra/test/clang-tidy/checkers/readability-inconsistent-declaration-parameter-name.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/readability-inconsistent-declaration-parameter-name.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-inconsistent-declaration-parameter-name.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s readability-inconsistent-declaration-parameter-name %t -- -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s readability-inconsistent-declaration-parameter-name %t -- --fix-notes -- -fno-delayed-template-parsing
void consistentFunction(int a, int b, int c);
void consistentFunction(int a, int b, int c);
Index: clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- -- -fno-delayed-template-parsing -isystem %S/Inputs/
-
+// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- --fix-notes -- -fno-delayed-template-parsing -isystem %S/Inputs/
// ----- Definitions -----
template <typename T> class vector {};
Index: clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls-cxx17.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls-cxx17.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls-cxx17.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++17-or-later %s misc-unused-using-decls %t -- -- -fno-delayed-template-parsing -isystem %S/Inputs/
+// RUN: %check_clang_tidy -std=c++17-or-later %s misc-unused-using-decls %t -- --fix-notes -- -fno-delayed-template-parsing -isystem %S/Inputs/
namespace ns {
Index: clang-tools-extra/test/clang-tidy/checkers/misc-definitions-in-headers.hpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/misc-definitions-in-headers.hpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-definitions-in-headers.hpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s misc-definitions-in-headers %t
+// RUN: %check_clang_tidy %s misc-definitions-in-headers %t -- --fix-notes
int f() {
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f' defined in a header file; function definitions in header files can lead to ODR violations [misc-definitions-in-headers]
Index: clang-tools-extra/docs/clang-tidy/index.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -173,6 +173,12 @@
errors were found. If compiler errors have
attached fix-its, clang-tidy will apply them as
well.
+ --fix-notes -
+ If a warning has no fix, but a single fix can
+ be found through an associated diagnostic note,
+ apply the fix.
+ Specifying this flag will implicitly enable the
+ '--fix' flag.
--format-style=<string> -
Style for formatting code around applied fixes:
- 'none' (default) turns off formatting
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -70,6 +70,10 @@
- The `run-clang-tidy.py` helper script is now installed in `bin/` as
`run-clang-tidy`. It was previously installed in `share/clang/`.
+- Added command line option `--fix-notes` to apply fixes found in notes
+ attached to warnings. These are typically cases where we are less confident
+ the fix will have the desired effect.
+
New checks
^^^^^^^^^^
Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===================================================================
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -127,6 +127,15 @@
)"),
cl::init(false), cl::cat(ClangTidyCategory));
+static cl::opt<bool> FixNotes("fix-notes", cl::desc(R"(
+If a warning has no fix, but a single fix can
+be found through an associated diagnostic note,
+apply the fix.
+Specifying this flag will implicitly enable the
+'--fix' flag.
+)"),
+ cl::init(false), cl::cat(ClangTidyCategory));
+
static cl::opt<std::string> FormatStyle("format-style", cl::desc(R"(
Style for formatting code around applied fixes:
- 'none' (default) turns off formatting
@@ -483,18 +492,22 @@
AllowEnablingAnalyzerAlphaCheckers);
std::vector<ClangTidyError> Errors =
runClangTidy(Context, OptionsParser->getCompilations(), PathList, BaseFS,
- EnableCheckProfile, ProfilePrefix);
+ FixNotes, EnableCheckProfile, ProfilePrefix);
bool FoundErrors = llvm::find_if(Errors, [](const ClangTidyError &E) {
return E.DiagLevel == ClangTidyError::Error;
}) != Errors.end();
- const bool DisableFixes = Fix && FoundErrors && !FixErrors;
+ // --fix-errors and --fix-notes imply --fix.
+ FixBehaviour Behaviour = FixNotes ? FB_FixNotes
+ : (Fix || FixErrors) ? FB_Fix
+ : FB_NoFix;
+
+ const bool DisableFixes = FoundErrors && !FixErrors;
unsigned WErrorCount = 0;
- // -fix-errors implies -fix.
- handleErrors(Errors, Context, (FixErrors || Fix) && !DisableFixes, WErrorCount,
- BaseFS);
+ handleErrors(Errors, Context, DisableFixes ? FB_NoFix : Behaviour,
+ WErrorCount, BaseFS);
if (!ExportFixes.empty() && !Errors.empty()) {
std::error_code EC;
@@ -508,7 +521,7 @@
if (!Quiet) {
printStats(Context.getStats());
- if (DisableFixes)
+ if (DisableFixes && Behaviour != FB_NoFix)
llvm::errs()
<< "Found compiler errors, but -fix-errors was not specified.\n"
"Fixes have NOT been applied.\n\n";
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -226,6 +226,13 @@
const Diagnostic &Info, ClangTidyContext &Context,
bool AllowIO = true);
+/// Gets the Fix attached to \p Diagnostic.
+/// If there isn't a Fix attached to the diagnostic and \p AnyFix is true, Check
+/// to see if exactly one note has a Fix and return it. Otherwise return
+/// nullptr.
+const llvm::StringMap<tooling::Replacements> *
+getFixIt(const tooling::Diagnostic &Diagnostic, bool AnyFix);
+
/// A diagnostic consumer that turns each \c Diagnostic into a
/// \c SourceManager-independent \c ClangTidyError.
//
@@ -235,7 +242,8 @@
public:
ClangTidyDiagnosticConsumer(ClangTidyContext &Ctx,
DiagnosticsEngine *ExternalDiagEngine = nullptr,
- bool RemoveIncompatibleErrors = true);
+ bool RemoveIncompatibleErrors = true,
+ bool GetFixesFromNotes = false);
// FIXME: The concept of converting between FixItHints and Replacements is
// more generic and should be pulled out into a more useful Diagnostics
@@ -265,6 +273,7 @@
ClangTidyContext &Context;
DiagnosticsEngine *ExternalDiagEngine;
bool RemoveIncompatibleErrors;
+ bool GetFixesFromNotes;
std::vector<ClangTidyError> Errors;
std::unique_ptr<llvm::Regex> HeaderFilter;
bool LastErrorRelatesToUserCode;
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -260,11 +260,11 @@
ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
ClangTidyContext &Ctx, DiagnosticsEngine *ExternalDiagEngine,
- bool RemoveIncompatibleErrors)
+ bool RemoveIncompatibleErrors, bool GetFixesFromNotes)
: Context(Ctx), ExternalDiagEngine(ExternalDiagEngine),
RemoveIncompatibleErrors(RemoveIncompatibleErrors),
- LastErrorRelatesToUserCode(false), LastErrorPassesLineFilter(false),
- LastErrorWasIgnored(false) {}
+ GetFixesFromNotes(GetFixesFromNotes), LastErrorRelatesToUserCode(false),
+ LastErrorPassesLineFilter(false), LastErrorWasIgnored(false) {}
void ClangTidyDiagnosticConsumer::finalizeLastError() {
if (!Errors.empty()) {
@@ -374,6 +374,24 @@
Context, AllowIO);
}
+const llvm::StringMap<tooling::Replacements> *
+getFixIt(const tooling::Diagnostic &Diagnostic, bool GetFixFromNotes) {
+ if (!Diagnostic.Message.Fix.empty())
+ return &Diagnostic.Message.Fix;
+ if (!GetFixFromNotes)
+ return nullptr;
+ const llvm::StringMap<tooling::Replacements> *Result = nullptr;
+ for (const auto &Note : Diagnostic.Notes) {
+ if (!Note.Fix.empty()) {
+ if (Result)
+ // We have 2 different fixes in notes, bail out.
+ return nullptr;
+ Result = &Note.Fix;
+ }
+ }
+ return Result;
+}
+
} // namespace tidy
} // namespace clang
@@ -658,7 +676,7 @@
std::pair<ClangTidyError *, llvm::StringMap<tooling::Replacements> *>>
ErrorFixes;
for (auto &Error : Errors) {
- if (const auto *Fix = tooling::selectFirstFix(Error))
+ if (const auto *Fix = getFixIt(Error, GetFixesFromNotes))
ErrorFixes.emplace_back(
&Error, const_cast<llvm::StringMap<tooling::Replacements> *>(Fix));
}
Index: clang-tools-extra/clang-tidy/ClangTidy.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidy.h
+++ clang-tools-extra/clang-tidy/ClangTidy.h
@@ -79,17 +79,28 @@
const tooling::CompilationDatabase &Compilations,
ArrayRef<std::string> InputFiles,
llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> BaseFS,
- bool EnableCheckProfile = false,
+ bool ApplyAnyFix, bool EnableCheckProfile = false,
llvm::StringRef StoreCheckProfile = StringRef());
+/// Controls what kind of fixes clang-tidy is allowed to apply.
+enum FixBehaviour {
+ /// Don't try to apply any fix.
+ FB_NoFix,
+ /// Only apply fixes added to warnings.
+ FB_Fix,
+ /// Apply fixes found in notes.
+ FB_FixNotes
+};
+
// FIXME: This interface will need to be significantly extended to be useful.
// FIXME: Implement confidence levels for displaying/fixing errors.
//
-/// Displays the found \p Errors to the users. If \p Fix is true, \p
-/// Errors containing fixes are automatically applied and reformatted. If no
-/// clang-format configuration file is found, the given \P FormatStyle is used.
+/// Displays the found \p Errors to the users. If \p Fix is \ref FB_Fix or \ref
+/// FB_FixNotes, \p Errors containing fixes are automatically applied and
+/// reformatted. If no clang-format configuration file is found, the given \P
+/// FormatStyle is used.
void handleErrors(llvm::ArrayRef<ClangTidyError> Errors,
- ClangTidyContext &Context, bool Fix,
+ ClangTidyContext &Context, FixBehaviour Fix,
unsigned &WarningsAsErrorsCount,
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS);
Index: clang-tools-extra/clang-tidy/ClangTidy.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -95,7 +95,7 @@
class ErrorReporter {
public:
- ErrorReporter(ClangTidyContext &Context, bool ApplyFixes,
+ ErrorReporter(ClangTidyContext &Context, FixBehaviour ApplyFixes,
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS)
: Files(FileSystemOptions(), std::move(BaseFS)),
DiagOpts(new DiagnosticOptions()),
@@ -133,8 +133,9 @@
auto Diag = Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]"))
<< Message.Message << Name;
// FIXME: explore options to support interactive fix selection.
- const llvm::StringMap<Replacements> *ChosenFix = selectFirstFix(Error);
- if (ApplyFixes && ChosenFix) {
+ const llvm::StringMap<Replacements> *ChosenFix;
+ if (ApplyFixes != FB_NoFix &&
+ (ChosenFix = getFixIt(Error, ApplyFixes == FB_FixNotes))) {
for (const auto &FileAndReplacements : *ChosenFix) {
for (const auto &Repl : FileAndReplacements.second) {
++TotalFixes;
@@ -187,7 +188,7 @@
}
void finish() {
- if (ApplyFixes && TotalFixes > 0) {
+ if (TotalFixes > 0) {
Rewriter Rewrite(SourceMgr, LangOpts);
for (const auto &FileAndReplacements : FileReplacements) {
StringRef File = FileAndReplacements.first();
@@ -287,7 +288,7 @@
SourceManager SourceMgr;
llvm::StringMap<Replacements> FileReplacements;
ClangTidyContext &Context;
- bool ApplyFixes;
+ FixBehaviour ApplyFixes;
unsigned TotalFixes;
unsigned AppliedFixes;
unsigned WarningsAsErrors;
@@ -500,7 +501,8 @@
const CompilationDatabase &Compilations,
ArrayRef<std::string> InputFiles,
llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> BaseFS,
- bool EnableCheckProfile, llvm::StringRef StoreCheckProfile) {
+ bool ApplyAnyFix, bool EnableCheckProfile,
+ llvm::StringRef StoreCheckProfile) {
ClangTool Tool(Compilations, InputFiles,
std::make_shared<PCHContainerOperations>(), BaseFS);
@@ -527,7 +529,7 @@
Context.setEnableProfiling(EnableCheckProfile);
Context.setProfileStoragePrefix(StoreCheckProfile);
- ClangTidyDiagnosticConsumer DiagConsumer(Context);
+ ClangTidyDiagnosticConsumer DiagConsumer(Context, nullptr, true, ApplyAnyFix);
DiagnosticsEngine DE(new DiagnosticIDs(), new DiagnosticOptions(),
&DiagConsumer, /*ShouldOwnClient=*/false);
Context.setDiagnosticsEngine(&DE);
@@ -574,7 +576,7 @@
}
void handleErrors(llvm::ArrayRef<ClangTidyError> Errors,
- ClangTidyContext &Context, bool Fix,
+ ClangTidyContext &Context, FixBehaviour Fix,
unsigned &WarningsAsErrorsCount,
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS) {
ErrorReporter Reporter(Context, Fix, std::move(BaseFS));
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
