njames93 updated this revision to Diff 313080.
njames93 marked 2 inline comments as done.
njames93 added a comment.
Use enum for handling fix behaviour.
Fix tests using `--fix-notes` instead of `-fix-notes`.
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/misc/UnusedUsingDeclsCheck.cpp
clang-tools-extra/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp
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.cpp
clang-tools-extra/test/clang-tidy/infrastructure/alternative-fixes.cpp
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,4 +1,4 @@
-// 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]
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
@@ -81,7 +81,6 @@
// eol-comments aren't removed (yet)
using n::A; // A
// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: using decl 'A' is unused
-// CHECK-MESSAGES: :[[@LINE-2]]:10: note: remove the using
// CHECK-FIXES: {{^}}// A
using n::B;
using n::C;
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,10 @@
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 has notes attached
+ which contain fixes, apply the first fix found
+ in any notes.
--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
@@ -67,6 +67,10 @@
Improvements to clang-tidy
--------------------------
+ - 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.
+
- Checks that allow configuring names of headers to include now support wrapping
the include in angle brackets to create a system include. For example,
:doc:`cppcoreguidelines-init-variables
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,13 @@
)"),
cl::init(false), cl::cat(ClangTidyCategory));
+static cl::opt<bool> FixNotes("fix-notes", cl::desc(R"(
+If a warning has no fix, but has notes attached
+which contain fixes, apply the first fix found
+in any notes.
+)"),
+ 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 +490,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 +519,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/readability/InconsistentDeclarationParameterNameCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp
@@ -219,21 +219,12 @@
return ParamInfo.SourceName;
};
- auto ParamDiag =
- Check->diag(Location,
- "differing parameters are named here: (%0), in %1: (%2)",
- DiagnosticIDs::Level::Note)
+ Check->diag(Location,
+ "differing parameters are named here: (%0), in %1: (%2)",
+ DiagnosticIDs::Level::Note)
<< joinParameterNames(DifferingParams, ChooseOtherName)
<< OtherDeclarationDescription
<< joinParameterNames(DifferingParams, ChooseSourceName);
-
- for (const DifferingParamInfo &ParamInfo : DifferingParams) {
- if (ParamInfo.GenerateFixItHint) {
- ParamDiag << FixItHint::CreateReplacement(
- CharSourceRange::getTokenRange(ParamInfo.OtherNameRange),
- ParamInfo.SourceName);
- }
- }
}
void formatDiagnosticsForDeclarations(
@@ -241,11 +232,24 @@
const FunctionDecl *ParameterSourceDeclaration,
const FunctionDecl *OriginalDeclaration,
const InconsistentDeclarationsContainer &InconsistentDeclarations) {
- Check->diag(
- OriginalDeclaration->getLocation(),
- "function %q0 has %1 other declaration%s1 with different parameter names")
- << OriginalDeclaration
- << static_cast<int>(InconsistentDeclarations.size());
+ {
+ auto MainDiag = Check->diag(OriginalDeclaration->getLocation(),
+ "function %q0 has %1 other declaration%s1 with "
+ "different parameter names")
+ << OriginalDeclaration
+ << static_cast<int>(InconsistentDeclarations.size());
+ for (const InconsistentDeclarationInfo &InconsistentDeclaration :
+ InconsistentDeclarations) {
+ for (const DifferingParamInfo &ParamInfo :
+ InconsistentDeclaration.DifferingParams) {
+ if (ParamInfo.GenerateFixItHint) {
+ MainDiag << FixItHint::CreateReplacement(
+ CharSourceRange::getTokenRange(ParamInfo.OtherNameRange),
+ ParamInfo.SourceName);
+ }
+ }
+ }
+ }
int Count = 1;
for (const InconsistentDeclarationInfo &InconsistentDeclaration :
InconsistentDeclarations) {
@@ -270,11 +274,21 @@
StringRef FunctionDescription, StringRef ParameterSourceDescription) {
for (const InconsistentDeclarationInfo &InconsistentDeclaration :
InconsistentDeclarations) {
- Check->diag(InconsistentDeclaration.DeclarationLocation,
- "%0 %q1 has a %2 with different parameter names")
- << FunctionDescription << OriginalDeclaration
- << ParameterSourceDescription;
-
+ {
+ auto MainDiag =
+ Check->diag(InconsistentDeclaration.DeclarationLocation,
+ "%0 %q1 has a %2 with different parameter names")
+ << FunctionDescription << OriginalDeclaration
+ << ParameterSourceDescription;
+ for (const DifferingParamInfo &ParamInfo :
+ InconsistentDeclaration.DifferingParams) {
+ if (ParamInfo.GenerateFixItHint) {
+ MainDiag << FixItHint::CreateReplacement(
+ CharSourceRange::getTokenRange(ParamInfo.OtherNameRange),
+ ParamInfo.SourceName);
+ }
+ }
+ }
Check->diag(ParameterSourceDeclaration->getLocation(), "the %0 seen here",
DiagnosticIDs::Level::Note)
<< ParameterSourceDescription;
Index: clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp
@@ -181,10 +181,7 @@
for (const auto &Context : Contexts) {
if (!Context.IsUsed) {
diag(Context.FoundUsingDecl->getLocation(), "using decl %0 is unused")
- << Context.FoundUsingDecl;
- // Emit a fix and a fix description of the check;
- diag(Context.FoundUsingDecl->getLocation(),
- /*Description=*/"remove the using", DiagnosticIDs::Note)
+ << Context.FoundUsingDecl
<< FixItHint::CreateRemoval(Context.UsingDeclRange);
}
}
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 FixIt attached to \p Diagnostic.
+/// If \p AnyFix is true and there is no FixIt attached to the Message,
+/// returns the first FixIt attached to any notes in the message.
+/// If no FixIt is found, returns 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 FindAnyFixIt = 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 FindAnyFixIt;
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
@@ -262,11 +262,11 @@
ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
ClangTidyContext &Ctx, DiagnosticsEngine *ExternalDiagEngine,
- bool RemoveIncompatibleErrors)
+ bool RemoveIncompatibleErrors, bool FindAnyFixit)
: Context(Ctx), ExternalDiagEngine(ExternalDiagEngine),
RemoveIncompatibleErrors(RemoveIncompatibleErrors),
- LastErrorRelatesToUserCode(false), LastErrorPassesLineFilter(false),
- LastErrorWasIgnored(false) {}
+ FindAnyFixIt(FindAnyFixit), LastErrorRelatesToUserCode(false),
+ LastErrorPassesLineFilter(false), LastErrorWasIgnored(false) {}
void ClangTidyDiagnosticConsumer::finalizeLastError() {
if (!Errors.empty()) {
@@ -376,6 +376,19 @@
Context, AllowIO);
}
+const llvm::StringMap<tooling::Replacements> *
+getFixIt(const tooling::Diagnostic &Diagnostic, bool AnyFix) {
+ if (!Diagnostic.Message.Fix.empty())
+ return &Diagnostic.Message.Fix;
+ if (AnyFix) {
+ for (const auto &Note : Diagnostic.Notes) {
+ if (!Note.Fix.empty())
+ return &Note.Fix;
+ }
+ }
+ return nullptr;
+}
+
} // namespace tidy
} // namespace clang
@@ -660,7 +673,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, FindAnyFixIt))
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
@@ -99,7 +99,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()),
@@ -137,8 +137,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;
@@ -191,7 +192,7 @@
}
void Finish() {
- if (ApplyFixes && TotalFixes > 0) {
+ if (TotalFixes > 0) {
Rewriter Rewrite(SourceMgr, LangOpts);
for (const auto &FileAndReplacements : FileReplacements) {
StringRef File = FileAndReplacements.first();
@@ -291,7 +292,7 @@
SourceManager SourceMgr;
llvm::StringMap<Replacements> FileReplacements;
ClangTidyContext &Context;
- bool ApplyFixes;
+ FixBehaviour ApplyFixes;
unsigned TotalFixes;
unsigned AppliedFixes;
unsigned WarningsAsErrors;
@@ -504,7 +505,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);
@@ -531,7 +533,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);
@@ -578,7 +580,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
