hokein updated this revision to Diff 193070.
hokein marked 2 inline comments as done.
hokein added a comment.
Update the patch:
- move FixDescriptions to tooling diagnostics, YAML serialization support
- add tests
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59932/new/
https://reviews.llvm.org/D59932
Files:
clang-tools-extra/clang-tidy/ClangTidy.cpp
clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
clang-tools-extra/clang-tidy/ClangTidyCheck.h
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tools-extra/test/clang-tidy/export-diagnostics.cpp
clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp
clang/include/clang/Tooling/Core/Diagnostic.h
clang/include/clang/Tooling/DiagnosticsYaml.h
clang/lib/Tooling/Core/Diagnostic.cpp
clang/unittests/Tooling/DiagnosticsYamlTest.cpp
Index: clang/unittests/Tooling/DiagnosticsYamlTest.cpp
===================================================================
--- clang/unittests/Tooling/DiagnosticsYamlTest.cpp
+++ clang/unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -31,9 +31,11 @@
static Diagnostic makeDiagnostic(StringRef DiagnosticName,
const std::string &Message, int FileOffset,
const std::string &FilePath,
- const StringMap<Replacements> &Fix) {
+ const StringMap<Replacements> &Fix,
+ const std::string &FixDescription) {
return Diagnostic(DiagnosticName, makeMessage(Message, FileOffset, FilePath),
- Fix, {}, Diagnostic::Warning, "path/to/build/directory");
+ Fix, {}, Diagnostic::Warning, "path/to/build/directory",
+ FixDescription);
}
TEST(DiagnosticsYamlTest, serializesDiagnostics) {
@@ -44,16 +46,18 @@
{"path/to/source.cpp",
Replacements({"path/to/source.cpp", 100, 12, "replacement #1"})}};
TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#1", "message #1", 55,
- "path/to/source.cpp", Fix1));
+ "path/to/source.cpp", Fix1,
+ "fix1 description"));
StringMap<Replacements> Fix2 = {
{"path/to/header.h",
Replacements({"path/to/header.h", 62, 2, "replacement #2"})}};
TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#2", "message #2", 60,
- "path/to/header.h", Fix2));
+ "path/to/header.h", Fix2,
+ "fix2 description"));
TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72,
- "path/to/source2.cpp", {}));
+ "path/to/source2.cpp", {}, ""));
TUD.Diagnostics.back().Notes.push_back(
makeMessage("Note1", 88, "path/to/note1.cpp"));
TUD.Diagnostics.back().Notes.push_back(
@@ -72,6 +76,7 @@
" Message: 'message #1'\n"
" FileOffset: 55\n"
" FilePath: 'path/to/source.cpp'\n"
+ " FixDescription: fix1 description\n"
" Replacements: \n"
" - FilePath: 'path/to/source.cpp'\n"
" Offset: 100\n"
@@ -81,6 +86,7 @@
" Message: 'message #2'\n"
" FileOffset: 60\n"
" FilePath: 'path/to/header.h'\n"
+ " FixDescription: fix2 description\n"
" Replacements: \n"
" - FilePath: 'path/to/header.h'\n"
" Offset: 62\n"
@@ -97,6 +103,7 @@
" - Message: Note2\n"
" FilePath: 'path/to/note2.cpp'\n"
" FileOffset: 99\n"
+ " FixDescription: ''\n"
" Replacements: []\n"
"...\n",
YamlContentStream.str());
@@ -110,6 +117,7 @@
" Message: 'message #1'\n"
" FileOffset: 55\n"
" FilePath: path/to/source.cpp\n"
+ " FixDescription: fix1 description\n"
" Replacements: \n"
" - FilePath: path/to/source.cpp\n"
" Offset: 100\n"
@@ -160,6 +168,7 @@
EXPECT_EQ("message #1", D1.Message.Message);
EXPECT_EQ(55u, D1.Message.FileOffset);
EXPECT_EQ("path/to/source.cpp", D1.Message.FilePath);
+ EXPECT_EQ("fix1 description", D1.FixDescription);
std::vector<Replacement> Fixes1 = getFixes(D1.Fix);
ASSERT_EQ(1u, Fixes1.size());
EXPECT_EQ("path/to/source.cpp", Fixes1[0].getFilePath());
Index: clang/lib/Tooling/Core/Diagnostic.cpp
===================================================================
--- clang/lib/Tooling/Core/Diagnostic.cpp
+++ clang/lib/Tooling/Core/Diagnostic.cpp
@@ -42,9 +42,11 @@
const DiagnosticMessage &Message,
const llvm::StringMap<Replacements> &Fix,
const SmallVector<DiagnosticMessage, 1> &Notes,
- Level DiagLevel, llvm::StringRef BuildDirectory)
+ Level DiagLevel, llvm::StringRef BuildDirectory,
+ llvm::StringRef FixDescription)
: DiagnosticName(DiagnosticName), Message(Message), Fix(Fix), Notes(Notes),
- DiagLevel(DiagLevel), BuildDirectory(BuildDirectory) {}
+ DiagLevel(DiagLevel), BuildDirectory(BuildDirectory),
+ FixDescription(FixDescription) {}
} // end namespace tooling
} // end namespace clang
Index: clang/include/clang/Tooling/DiagnosticsYaml.h
===================================================================
--- clang/include/clang/Tooling/DiagnosticsYaml.h
+++ clang/include/clang/Tooling/DiagnosticsYaml.h
@@ -45,11 +45,12 @@
NormalizedDiagnostic(const IO &, const clang::tooling::Diagnostic &D)
: DiagnosticName(D.DiagnosticName), Message(D.Message), Fix(D.Fix),
Notes(D.Notes), DiagLevel(D.DiagLevel),
- BuildDirectory(D.BuildDirectory) {}
+ BuildDirectory(D.BuildDirectory), FixDescription(D.FixDescription) {}
clang::tooling::Diagnostic denormalize(const IO &) {
return clang::tooling::Diagnostic(DiagnosticName, Message, Fix, Notes,
- DiagLevel, BuildDirectory);
+ DiagLevel, BuildDirectory,
+ FixDescription);
}
std::string DiagnosticName;
@@ -58,6 +59,7 @@
SmallVector<clang::tooling::DiagnosticMessage, 1> Notes;
clang::tooling::Diagnostic::Level DiagLevel;
std::string BuildDirectory;
+ std::string FixDescription;
};
static void mapping(IO &Io, clang::tooling::Diagnostic &D) {
@@ -77,6 +79,7 @@
Fixes.push_back(Replacement);
}
}
+ Io.mapOptional("FixDescription", Keys->FixDescription);
Io.mapRequired("Replacements", Fixes);
for (auto &Fix : Fixes) {
llvm::Error Err = Keys->Fix[Fix.getFilePath()].add(Fix);
Index: clang/include/clang/Tooling/Core/Diagnostic.h
===================================================================
--- clang/include/clang/Tooling/Core/Diagnostic.h
+++ clang/include/clang/Tooling/Core/Diagnostic.h
@@ -60,7 +60,7 @@
Diagnostic(llvm::StringRef DiagnosticName, const DiagnosticMessage &Message,
const llvm::StringMap<Replacements> &Fix,
const SmallVector<DiagnosticMessage, 1> &Notes, Level DiagLevel,
- llvm::StringRef BuildDirectory);
+ llvm::StringRef BuildDirectory, llvm::StringRef FixDescription);
/// Name identifying the Diagnostic.
std::string DiagnosticName;
@@ -85,6 +85,10 @@
///
/// Note: it is empty in unittest.
std::string BuildDirectory;
+
+ // A short descrpition of the clang-tidy fix.
+ // Empty if the Diagnostic doesn't provide any fixes.
+ std::string FixDescription;
};
/// Collection of Diagnostics generated from a single translation unit.
Index: clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp
===================================================================
--- clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp
@@ -1,5 +1,6 @@
#include "ClangTidy.h"
#include "ClangTidyTest.h"
+#include "clang/Basic/Diagnostic.h"
#include "gtest/gtest.h"
namespace clang {
@@ -13,10 +14,12 @@
void registerMatchers(ast_matchers::MatchFinder *Finder) override {
Finder->addMatcher(ast_matchers::varDecl().bind("var"), this);
}
+ llvm::StringRef fixDescription() override { return "test fix description"; }
void check(const ast_matchers::MatchFinder::MatchResult &Result) override {
const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var");
// Add diagnostics in the wrong order.
- diag(Var->getLocation(), "variable");
+ diag(Var->getLocation(), "variable")
+ << FixItHint::CreateInsertion(Var->getBeginLoc(), "/*empty*/");
diag(Var->getTypeSpecStartLoc(), "type specifier");
}
};
@@ -27,6 +30,7 @@
EXPECT_EQ(2ul, Errors.size());
EXPECT_EQ("type specifier", Errors[0].Message.Message);
EXPECT_EQ("variable", Errors[1].Message.Message);
+ EXPECT_EQ("test fix description", Errors[1].FixDescription);
}
TEST(GlobList, Empty) {
Index: clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
===================================================================
--- clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
+++ clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
@@ -21,7 +21,7 @@
makeTUDiagnostics(const std::string &MainSourceFile, StringRef DiagnosticName,
const DiagnosticMessage &Message,
const StringMap<Replacements> &Replacements,
- StringRef BuildDirectory) {
+ StringRef BuildDirectory, StringRef FixDescription) {
TUDiagnostics TUs;
TUs.push_back({MainSourceFile,
{{DiagnosticName,
@@ -29,7 +29,8 @@
Replacements,
{},
Diagnostic::Warning,
- BuildDirectory}}});
+ BuildDirectory,
+ FixDescription}}});
return TUs;
}
@@ -42,8 +43,8 @@
FileManager Files((FileSystemOptions()));
SourceManager SM(Diagnostics, Files);
TUReplacements TURs;
- TUDiagnostics TUs =
- makeTUDiagnostics("path/to/source.cpp", "diagnostic", {}, {}, "path/to");
+ TUDiagnostics TUs = makeTUDiagnostics("path/to/source.cpp", "diagnostic", {},
+ {}, "path/to", "");
FileToChangesMap ReplacementsMap;
EXPECT_TRUE(mergeAndDeduplicate(TURs, TUs, ReplacementsMap, SM));
Index: clang-tools-extra/test/clang-tidy/export-diagnostics.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/export-diagnostics.cpp
+++ clang-tools-extra/test/clang-tidy/export-diagnostics.cpp
@@ -23,6 +23,7 @@
// CHECK-YAML-NEXT: - Message: expanded from here
// CHECK-YAML-NEXT: FilePath: ''
// CHECK-YAML-NEXT: FileOffset: 0
+// CHECK-YAML-NEXT: FixDescription: ''
// CHECK-YAML-NEXT: Replacements: []
// CHECK-YAML-NEXT: ...
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/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"
@@ -116,7 +117,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 +141,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 +214,9 @@
std::string CurrentBuildDirectory;
- llvm::DenseMap<unsigned, std::string> CheckNamesByDiagnosticID;
+ // A map from diagnostic id to <check_name, check_fix_description>.
+ llvm::DenseMap<unsigned, std::pair<std::string, std::string>>
+ CheckNamesByDiagnosticID;
bool Profile;
std::string ProfilePrefix;
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/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-tools-extra/clang-tidy/ClangTidyCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyCheck.h
+++ clang-tools-extra/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-tools-extra/clang-tidy/ClangTidyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
+++ clang-tools-extra/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-tools-extra/clang-tidy/ClangTidy.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ clang-tools-extra/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
