Author: hokein
Date: Wed Apr 17 05:53:59 2019
New Revision: 358576
URL: http://llvm.org/viewvc/llvm-project?rev=358576&view=rev
Log:
[clang-tidy] Add fix descriptions to clang-tidy checks.
Summary:
Motivation/Context: in the code review system integrating with clang-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.
This patch extends clang-tidy framework to support fix descriptions (will add
implementations for
existing checks in the future). Fix descriptions and fixes are emitted via
diagnostic::Note (rather than
attaching the main warning diagnostic).
Before this patch:
```
void MyCheck::check(...) {
...
diag(loc, "my check warning") << FixtItHint::CreateReplacement(...);
}
```
After:
```
void MyCheck::check(...) {
...
diag(loc, "my check warning"); // Emit a check warning
diag(loc, "fix description", DiagnosticIDs::Note) <<
FixtItHint::CreateReplacement(...); // Emit a diagnostic note and a fix
}
```
Reviewers: sammccall, alexfh
Reviewed By: alexfh
Subscribers: MyDeveloperDay, Eugene.Zelenko, aaron.ballman, JonasToth,
xazax.hun, jdoerfert, cfe-commits
Tags: #clang-tools-extra, #clang
Differential Revision: https://reviews.llvm.org/D59932
Modified:
cfe/trunk/include/clang/Tooling/Core/Diagnostic.h
cfe/trunk/include/clang/Tooling/DiagnosticsYaml.h
cfe/trunk/lib/Tooling/Core/Diagnostic.cpp
cfe/trunk/unittests/Tooling/DiagnosticsYamlTest.cpp
Modified: cfe/trunk/include/clang/Tooling/Core/Diagnostic.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Core/Diagnostic.h?rev=358576&r1=358575&r2=358576&view=diff
==============================================================================
--- cfe/trunk/include/clang/Tooling/Core/Diagnostic.h (original)
+++ cfe/trunk/include/clang/Tooling/Core/Diagnostic.h Wed Apr 17 05:53:59 2019
@@ -42,6 +42,9 @@ struct DiagnosticMessage {
std::string Message;
std::string FilePath;
unsigned FileOffset;
+
+ /// Fixes for this diagnostic, grouped by file path.
+ llvm::StringMap<Replacements> Fix;
};
/// Represents the diagnostic with the level of severity and possible
@@ -58,7 +61,6 @@ struct Diagnostic {
StringRef BuildDirectory);
Diagnostic(llvm::StringRef DiagnosticName, const DiagnosticMessage &Message,
- const llvm::StringMap<Replacements> &Fix,
const SmallVector<DiagnosticMessage, 1> &Notes, Level DiagLevel,
llvm::StringRef BuildDirectory);
@@ -68,9 +70,6 @@ struct Diagnostic {
/// Message associated to the diagnostic.
DiagnosticMessage Message;
- /// Fixes to apply, grouped by file path.
- llvm::StringMap<Replacements> Fix;
-
/// Potential notes about the diagnostic.
SmallVector<DiagnosticMessage, 1> Notes;
@@ -94,6 +93,10 @@ struct TranslationUnitDiagnostics {
std::vector<Diagnostic> Diagnostics;
};
+// Get the first fix to apply for this diagnostic.
+// Return nullptr if no fixes attached to the diagnostic.
+const llvm::StringMap<Replacements> *selectFirstFix(const Diagnostic& D);
+
} // end namespace tooling
} // end namespace clang
#endif // LLVM_CLANG_TOOLING_CORE_DIAGNOSTIC_H
Modified: cfe/trunk/include/clang/Tooling/DiagnosticsYaml.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/DiagnosticsYaml.h?rev=358576&r1=358575&r2=358576&view=diff
==============================================================================
--- cfe/trunk/include/clang/Tooling/DiagnosticsYaml.h (original)
+++ cfe/trunk/include/clang/Tooling/DiagnosticsYaml.h Wed Apr 17 05:53:59 2019
@@ -31,6 +31,20 @@ template <> struct MappingTraits<clang::
Io.mapRequired("Message", M.Message);
Io.mapOptional("FilePath", M.FilePath);
Io.mapOptional("FileOffset", M.FileOffset);
+ std::vector<clang::tooling::Replacement> Fixes;
+ for (auto &Replacements : M.Fix) {
+ for (auto &Replacement : Replacements.second)
+ Fixes.push_back(Replacement);
+ }
+ Io.mapRequired("Replacements", Fixes);
+ for (auto &Fix : Fixes) {
+ llvm::Error Err = M.Fix[Fix.getFilePath()].add(Fix);
+ if (Err) {
+ // FIXME: Implement better conflict handling.
+ llvm::errs() << "Fix conflicts with existing fix: "
+ << llvm::toString(std::move(Err)) << "\n";
+ }
+ }
}
};
@@ -43,12 +57,11 @@ template <> struct MappingTraits<clang::
: DiagLevel(clang::tooling::Diagnostic::Level::Warning) {}
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) {}
+ : DiagnosticName(D.DiagnosticName), Message(D.Message), Notes(D.Notes),
+ DiagLevel(D.DiagLevel), BuildDirectory(D.BuildDirectory) {}
clang::tooling::Diagnostic denormalize(const IO &) {
- return clang::tooling::Diagnostic(DiagnosticName, Message, Fix, Notes,
+ return clang::tooling::Diagnostic(DiagnosticName, Message, Notes,
DiagLevel, BuildDirectory);
}
@@ -64,28 +77,10 @@ template <> struct MappingTraits<clang::
MappingNormalization<NormalizedDiagnostic, clang::tooling::Diagnostic>
Keys(
Io, D);
Io.mapRequired("DiagnosticName", Keys->DiagnosticName);
- Io.mapRequired("Message", Keys->Message.Message);
- Io.mapRequired("FileOffset", Keys->Message.FileOffset);
- Io.mapRequired("FilePath", Keys->Message.FilePath);
+ Io.mapRequired("DiagnosticMessage", Keys->Message);
Io.mapOptional("Notes", Keys->Notes);
// FIXME: Export properly all the different fields.
-
- std::vector<clang::tooling::Replacement> Fixes;
- for (auto &Replacements : Keys->Fix) {
- for (auto &Replacement : Replacements.second) {
- Fixes.push_back(Replacement);
- }
- }
- Io.mapRequired("Replacements", Fixes);
- for (auto &Fix : Fixes) {
- llvm::Error Err = Keys->Fix[Fix.getFilePath()].add(Fix);
- if (Err) {
- // FIXME: Implement better conflict handling.
- llvm::errs() << "Fix conflicts with existing fix: "
- << llvm::toString(std::move(Err)) << "\n";
- }
- }
}
};
Modified: cfe/trunk/lib/Tooling/Core/Diagnostic.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Core/Diagnostic.cpp?rev=358576&r1=358575&r2=358576&view=diff
==============================================================================
--- cfe/trunk/lib/Tooling/Core/Diagnostic.cpp (original)
+++ cfe/trunk/lib/Tooling/Core/Diagnostic.cpp Wed Apr 17 05:53:59 2019
@@ -12,6 +12,7 @@
#include "clang/Tooling/Core/Diagnostic.h"
#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/STLExtras.h"
namespace clang {
namespace tooling {
@@ -40,11 +41,21 @@ Diagnostic::Diagnostic(llvm::StringRef D
Diagnostic::Diagnostic(llvm::StringRef DiagnosticName,
const DiagnosticMessage &Message,
- const llvm::StringMap<Replacements> &Fix,
const SmallVector<DiagnosticMessage, 1> &Notes,
Level DiagLevel, llvm::StringRef BuildDirectory)
- : DiagnosticName(DiagnosticName), Message(Message), Fix(Fix), Notes(Notes),
+ : DiagnosticName(DiagnosticName), Message(Message), Notes(Notes),
DiagLevel(DiagLevel), BuildDirectory(BuildDirectory) {}
+const llvm::StringMap<Replacements> *selectFirstFix(const Diagnostic& D) {
+ if (!D.Message.Fix.empty())
+ return &D.Message.Fix;
+ auto Iter = llvm::find_if(D.Notes, [](const tooling::DiagnosticMessage &D) {
+ return !D.Fix.empty();
+ });
+ if (Iter != D.Notes.end())
+ return &Iter->Fix;
+ return nullptr;
+}
+
} // end namespace tooling
} // end namespace clang
Modified: cfe/trunk/unittests/Tooling/DiagnosticsYamlTest.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/DiagnosticsYamlTest.cpp?rev=358576&r1=358575&r2=358576&view=diff
==============================================================================
--- cfe/trunk/unittests/Tooling/DiagnosticsYamlTest.cpp (original)
+++ cfe/trunk/unittests/Tooling/DiagnosticsYamlTest.cpp Wed Apr 17 05:53:59 2019
@@ -20,11 +20,13 @@ using namespace clang::tooling;
using clang::tooling::Diagnostic;
static DiagnosticMessage makeMessage(const std::string &Message, int
FileOffset,
- const std::string &FilePath) {
+ const std::string &FilePath,
+ const StringMap<Replacements> &Fix) {
DiagnosticMessage DiagMessage;
DiagMessage.Message = Message;
DiagMessage.FileOffset = FileOffset;
DiagMessage.FilePath = FilePath;
+ DiagMessage.Fix = Fix;
return DiagMessage;
}
@@ -32,10 +34,52 @@ static Diagnostic makeDiagnostic(StringR
const std::string &Message, int FileOffset,
const std::string &FilePath,
const StringMap<Replacements> &Fix) {
- return Diagnostic(DiagnosticName, makeMessage(Message, FileOffset, FilePath),
- Fix, {}, Diagnostic::Warning, "path/to/build/directory");
+ return Diagnostic(DiagnosticName,
+ makeMessage(Message, FileOffset, FilePath, Fix), {},
+ Diagnostic::Warning, "path/to/build/directory");
}
+static const char *YAMLContent =
+ "---\n"
+ "MainSourceFile: 'path/to/source.cpp'\n"
+ "Diagnostics: \n"
+ " - DiagnosticName: 'diagnostic#1\'\n"
+ " DiagnosticMessage: \n"
+ " Message: 'message #1'\n"
+ " FilePath: 'path/to/source.cpp'\n"
+ " FileOffset: 55\n"
+ " Replacements: \n"
+ " - FilePath: 'path/to/source.cpp'\n"
+ " Offset: 100\n"
+ " Length: 12\n"
+ " ReplacementText: 'replacement #1'\n"
+ " - DiagnosticName: 'diagnostic#2'\n"
+ " DiagnosticMessage: \n"
+ " Message: 'message #2'\n"
+ " FilePath: 'path/to/header.h'\n"
+ " FileOffset: 60\n"
+ " Replacements: \n"
+ " - FilePath: 'path/to/header.h'\n"
+ " Offset: 62\n"
+ " Length: 2\n"
+ " ReplacementText: 'replacement #2'\n"
+ " - DiagnosticName: 'diagnostic#3'\n"
+ " DiagnosticMessage: \n"
+ " Message: 'message #3'\n"
+ " FilePath: 'path/to/source2.cpp'\n"
+ " FileOffset: 72\n"
+ " Replacements: []\n"
+ " Notes: \n"
+ " - Message: Note1\n"
+ " FilePath: 'path/to/note1.cpp'\n"
+ " FileOffset: 88\n"
+ " Replacements: []\n"
+ " - Message: Note2\n"
+ " FilePath: 'path/to/note2.cpp'\n"
+ " FileOffset: 99\n"
+ " Replacements: []\n"
+ "...\n";
+
TEST(DiagnosticsYamlTest, serializesDiagnostics) {
TranslationUnitDiagnostics TUD;
TUD.MainSourceFile = "path/to/source.cpp";
@@ -55,9 +99,9 @@ TEST(DiagnosticsYamlTest, serializesDiag
TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72,
"path/to/source2.cpp", {}));
TUD.Diagnostics.back().Notes.push_back(
- makeMessage("Note1", 88, "path/to/note1.cpp"));
+ makeMessage("Note1", 88, "path/to/note1.cpp", {}));
TUD.Diagnostics.back().Notes.push_back(
- makeMessage("Note2", 99, "path/to/note2.cpp"));
+ makeMessage("Note2", 99, "path/to/note2.cpp", {}));
std::string YamlContent;
raw_string_ostream YamlContentStream(YamlContent);
@@ -65,80 +109,12 @@ TEST(DiagnosticsYamlTest, serializesDiag
yaml::Output YAML(YamlContentStream);
YAML << TUD;
- EXPECT_EQ("---\n"
- "MainSourceFile: 'path/to/source.cpp'\n"
- "Diagnostics: \n"
- " - DiagnosticName: 'diagnostic#1\'\n"
- " Message: 'message #1'\n"
- " FileOffset: 55\n"
- " FilePath: 'path/to/source.cpp'\n"
- " Replacements: \n"
- " - FilePath: 'path/to/source.cpp'\n"
- " Offset: 100\n"
- " Length: 12\n"
- " ReplacementText: 'replacement #1'\n"
- " - DiagnosticName: 'diagnostic#2'\n"
- " Message: 'message #2'\n"
- " FileOffset: 60\n"
- " FilePath: 'path/to/header.h'\n"
- " Replacements: \n"
- " - FilePath: 'path/to/header.h'\n"
- " Offset: 62\n"
- " Length: 2\n"
- " ReplacementText: 'replacement #2'\n"
- " - DiagnosticName: 'diagnostic#3'\n"
- " Message: 'message #3'\n"
- " FileOffset: 72\n"
- " FilePath: 'path/to/source2.cpp'\n"
- " Notes: \n"
- " - Message: Note1\n"
- " FilePath: 'path/to/note1.cpp'\n"
- " FileOffset: 88\n"
- " - Message: Note2\n"
- " FilePath: 'path/to/note2.cpp'\n"
- " FileOffset: 99\n"
- " Replacements: []\n"
- "...\n",
- YamlContentStream.str());
+ EXPECT_EQ(YAMLContent, YamlContentStream.str());
}
TEST(DiagnosticsYamlTest, deserializesDiagnostics) {
- std::string YamlContent = "---\n"
- "MainSourceFile: path/to/source.cpp\n"
- "Diagnostics: \n"
- " - DiagnosticName: 'diagnostic#1'\n"
- " Message: 'message #1'\n"
- " FileOffset: 55\n"
- " FilePath: path/to/source.cpp\n"
- " Replacements: \n"
- " - FilePath: path/to/source.cpp\n"
- " Offset: 100\n"
- " Length: 12\n"
- " ReplacementText: 'replacement #1'\n"
- " - DiagnosticName: 'diagnostic#2'\n"
- " Message: 'message #2'\n"
- " FileOffset: 60\n"
- " FilePath: path/to/header.h\n"
- " Replacements: \n"
- " - FilePath: path/to/header.h\n"
- " Offset: 62\n"
- " Length: 2\n"
- " ReplacementText: 'replacement #2'\n"
- " - DiagnosticName: 'diagnostic#3'\n"
- " Message: 'message #3'\n"
- " FileOffset: 98\n"
- " FilePath: path/to/source.cpp\n"
- " Notes:\n"
- " - Message: Note1\n"
- " FilePath: 'path/to/note1.cpp'\n"
- " FileOffset: 66\n"
- " - Message: Note2\n"
- " FilePath: 'path/to/note2.cpp'\n"
- " FileOffset: 77\n"
- " Replacements: []\n"
- "...\n";
TranslationUnitDiagnostics TUDActual;
- yaml::Input YAML(YamlContent);
+ yaml::Input YAML(YAMLContent);
YAML >> TUDActual;
ASSERT_FALSE(YAML.error());
@@ -160,7 +136,7 @@ TEST(DiagnosticsYamlTest, deserializesDi
EXPECT_EQ("message #1", D1.Message.Message);
EXPECT_EQ(55u, D1.Message.FileOffset);
EXPECT_EQ("path/to/source.cpp", D1.Message.FilePath);
- std::vector<Replacement> Fixes1 = getFixes(D1.Fix);
+ std::vector<Replacement> Fixes1 = getFixes(D1.Message.Fix);
ASSERT_EQ(1u, Fixes1.size());
EXPECT_EQ("path/to/source.cpp", Fixes1[0].getFilePath());
EXPECT_EQ(100u, Fixes1[0].getOffset());
@@ -172,7 +148,7 @@ TEST(DiagnosticsYamlTest, deserializesDi
EXPECT_EQ("message #2", D2.Message.Message);
EXPECT_EQ(60u, D2.Message.FileOffset);
EXPECT_EQ("path/to/header.h", D2.Message.FilePath);
- std::vector<Replacement> Fixes2 = getFixes(D2.Fix);
+ std::vector<Replacement> Fixes2 = getFixes(D2.Message.Fix);
ASSERT_EQ(1u, Fixes2.size());
EXPECT_EQ("path/to/header.h", Fixes2[0].getFilePath());
EXPECT_EQ(62u, Fixes2[0].getOffset());
@@ -182,15 +158,15 @@ TEST(DiagnosticsYamlTest, deserializesDi
Diagnostic D3 = TUDActual.Diagnostics[2];
EXPECT_EQ("diagnostic#3", D3.DiagnosticName);
EXPECT_EQ("message #3", D3.Message.Message);
- EXPECT_EQ(98u, D3.Message.FileOffset);
- EXPECT_EQ("path/to/source.cpp", D3.Message.FilePath);
+ EXPECT_EQ(72u, D3.Message.FileOffset);
+ EXPECT_EQ("path/to/source2.cpp", D3.Message.FilePath);
EXPECT_EQ(2u, D3.Notes.size());
EXPECT_EQ("Note1", D3.Notes[0].Message);
- EXPECT_EQ(66u, D3.Notes[0].FileOffset);
+ EXPECT_EQ(88u, D3.Notes[0].FileOffset);
EXPECT_EQ("path/to/note1.cpp", D3.Notes[0].FilePath);
EXPECT_EQ("Note2", D3.Notes[1].Message);
- EXPECT_EQ(77u, D3.Notes[1].FileOffset);
+ EXPECT_EQ(99u, D3.Notes[1].FileOffset);
EXPECT_EQ("path/to/note2.cpp", D3.Notes[1].FilePath);
- std::vector<Replacement> Fixes3 = getFixes(D3.Fix);
+ std::vector<Replacement> Fixes3 = getFixes(D3.Message.Fix);
EXPECT_TRUE(Fixes3.empty());
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
