hokein created this revision.
Herald added subscribers: cfe-commits, jdoerfert, xazax.hun.
Herald added a project: clang.
Motivation/Context: in the code review system integrating withclang-tidy,
clang-tidy doesn't provide a human-readable description of the fix. Usually
developers have to preview a code diff (before vs after apply the fix) to
understand what the fix does before applying a fix.
This patch proposes that each clang-tidy check provides a short and
actional fix description that can be shown in the UI, so that users can know
what the fix does without previewing diff.
We introduce a new interface to ClangTidyCheck, we will add implementations for
exisiting checks.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D59932
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidyCheck.cpp
clang-tidy/ClangTidyCheck.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
Index: clang-tidy/ClangTidyDiagnosticConsumer.h
===================================================================
--- clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -16,6 +16,7 @@
#include "clang/Tooling/Core/Diagnostic.h"
#include "clang/Tooling/Refactoring.h"
#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseMapInfo.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/Support/Regex.h"
#include "llvm/Support/Timer.h"
@@ -45,6 +46,11 @@
bool IsWarningAsError);
bool IsWarningAsError;
+ // A short descrpition of the clang-tidy fix.
+ // Empty if the ClangTidyError doesn't provide any fixes.
+ //
+ // FIXME: this should move to tooling::Diagnostic.
+ std::string FixDescription;
};
/// \brief Read-only set of strings represented as a list of positive and
@@ -116,7 +122,7 @@
/// tablegen'd diagnostic IDs.
/// FIXME: Figure out a way to manage ID spaces.
DiagnosticBuilder diag(StringRef CheckName, SourceLocation Loc,
- StringRef Message,
+ StringRef Message, StringRef CheckFixDescription,
DiagnosticIDs::Level Level = DiagnosticIDs::Warning);
/// \brief Sets the \c SourceManager of the used \c DiagnosticsEngine.
@@ -140,6 +146,10 @@
/// diagnostic ID.
std::string getCheckName(unsigned DiagnosticID) const;
+ /// \brief Returns the fix description of the clang-tidy check which produced
+ /// this diagnostic ID.
+ std::string getCheckFixDescription(unsigned DiagnosticID) const;
+
/// \brief Returns \c true if the check is enabled for the \c CurrentFile.
///
/// The \c CurrentFile can be changed using \c setCurrentFile.
@@ -209,7 +219,8 @@
std::string CurrentBuildDirectory;
- llvm::DenseMap<unsigned, std::string> CheckNamesByDiagnosticID;
+ llvm::DenseMap<unsigned, std::pair<std::string, std::string>>
+ CheckNamesByDiagnosticID;
bool Profile;
std::string ProfilePrefix;
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===================================================================
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -190,11 +190,12 @@
DiagnosticBuilder ClangTidyContext::diag(
StringRef CheckName, SourceLocation Loc, StringRef Description,
+ StringRef CheckFixDescription,
DiagnosticIDs::Level Level /* = DiagnosticIDs::Warning*/) {
assert(Loc.isValid());
unsigned ID = DiagEngine->getDiagnosticIDs()->getCustomDiagID(
Level, (Description + " [" + CheckName + "]").str());
- CheckNamesByDiagnosticID.try_emplace(ID, CheckName);
+ CheckNamesByDiagnosticID[ID] = {CheckName, CheckFixDescription};
return DiagEngine->Report(Loc, ID);
}
@@ -259,10 +260,17 @@
DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(DiagnosticID);
if (!ClangWarningOption.empty())
return "clang-diagnostic-" + ClangWarningOption;
- llvm::DenseMap<unsigned, std::string>::const_iterator I =
- CheckNamesByDiagnosticID.find(DiagnosticID);
+ auto I = CheckNamesByDiagnosticID.find(DiagnosticID);
if (I != CheckNamesByDiagnosticID.end())
- return I->second;
+ return I->second.first;
+ return "";
+}
+
+std::string
+ClangTidyContext::getCheckFixDescription(unsigned DiagnosticID) const {
+ auto I = CheckNamesByDiagnosticID.find(DiagnosticID);
+ if (I != CheckNamesByDiagnosticID.end())
+ return I->second.second;
return "";
}
@@ -449,6 +457,9 @@
Loc = FullSourceLoc(Info.getLocation(), Info.getSourceManager());
Converter.emitDiagnostic(Loc, DiagLevel, Message, Info.getRanges(),
Info.getFixItHints());
+ // Populate the fix description if the diagnostic has fixes.
+ if (!Errors.back().Fix.empty())
+ Errors.back().FixDescription = Context.getCheckFixDescription(Info.getID());
if (Info.hasSourceManager())
checkFilters(Info.getLocation(), Info.getSourceManager());
Index: clang-tidy/ClangTidyCheck.h
===================================================================
--- clang-tidy/ClangTidyCheck.h
+++ clang-tidy/ClangTidyCheck.h
@@ -92,6 +92,22 @@
/// work in here.
virtual void check(const ast_matchers::MatchFinder::MatchResult &Result) {}
+ /// A short human readable description about the check fix. It should
+ /// descripe what the check fix will do, so users can rely on it to know the
+ /// fix intention without viewing the actual code change.
+ ///
+ /// Example: "Convert to 'switch'"; "Removed unused using"; "Use make_unique".
+ ///
+ /// The description will be attached to ``ClangTidyError``. Returning empty
+ /// string indicates the check doesn't provide this information.
+ ///
+ /// Caveats:
+ /// - The interface may not fit well with a check that produces different
+ /// fixes for different cases.
+ /// - clang compiler diagnostics and clang static analyzer diagnostics are
+ /// not supported.
+ virtual llvm::StringRef fixDescription() { return ""; }
+
/// \brief Add a diagnostic with the check's name.
DiagnosticBuilder diag(SourceLocation Loc, StringRef Description,
DiagnosticIDs::Level Level = DiagnosticIDs::Warning);
Index: clang-tidy/ClangTidyCheck.cpp
===================================================================
--- clang-tidy/ClangTidyCheck.cpp
+++ clang-tidy/ClangTidyCheck.cpp
@@ -20,7 +20,7 @@
DiagnosticBuilder ClangTidyCheck::diag(SourceLocation Loc, StringRef Message,
DiagnosticIDs::Level Level) {
- return Context->diag(CheckName, Loc, Message, Level);
+ return Context->diag(CheckName, Loc, Message, fixDescription(), Level);
}
void ClangTidyCheck::run(const ast_matchers::MatchFinder::MatchResult &Result) {
Index: clang-tidy/ClangTidy.cpp
===================================================================
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -72,13 +72,14 @@
SmallString<64> CheckName(AnalyzerCheckNamePrefix);
CheckName += PD->getCheckName();
Context.diag(CheckName, PD->getLocation().asLocation(),
- PD->getShortDescription())
+ PD->getShortDescription(), /*FixDescription*/ "")
<< PD->path.back()->getRanges();
for (const auto &DiagPiece :
PD->path.flatten(/*ShouldFlattenMacros=*/true)) {
Context.diag(CheckName, DiagPiece->getLocation().asLocation(),
- DiagPiece->getString(), DiagnosticIDs::Note)
+ DiagPiece->getString(), /*FixDescription*/ "",
+ DiagnosticIDs::Note)
<< DiagPiece->getRanges();
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
