njames93 created this revision.
njames93 added reviewers: alexfh, aaron.ballman.
Herald added subscribers: usaxena95, kadircet, arphaman, xazax.hun.
njames93 published this revision for review.
njames93 added a subscriber: NoQ.
njames93 added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
@NoQ This may have a little impact on D95403 <https://reviews.llvm.org/D95403>,
but from what I can see nothing mayor would conflict. It was fairly easy to fix
up clang d integration.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:170-171
+ auto Res = DiagEngine->Report(Loc, ID);
+ // Repurpse the Flag value to store the check name. clang-tidy checks don't
+ // have command line flags so this field is safe ro use, but add a prefix
just
+ // to be sure.
----------------
Repurpose the FlagValue field in DiagnosticEngine to store the Checkname.
This removes the need to remove the checkname from the message when we process
the diagnostic later.
I looked at a few options and FlagValue seems the simplest way to implement
this.
Its only ever read when there is a warning option for the given DiagnosticID,
which will never happen for clang-tidy checks.
Removing the Checkname is also a necessary first step if we ever want to
support tablegen diagnostics route for clang-tidy.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D97415
Files:
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tools-extra/clangd/Diagnostics.cpp
clang-tools-extra/clangd/ParsedAST.cpp
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -323,9 +323,10 @@
isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress))
return DiagnosticsEngine::Ignored;
if (!CTChecks.empty()) {
- std::string CheckName = CTContext->getCheckName(Info.getID());
- bool IsClangTidyDiag = !CheckName.empty();
- if (IsClangTidyDiag) {
+ SmallString<64> Buffer;
+ auto CheckName =
+ tidy::ClangTidyContext::getDiagnosticName(Info, Buffer);
+ if (!CheckName.empty()) {
if (Cfg.Diagnostics.Suppress.contains(CheckName))
return DiagnosticsEngine::Ignored;
// Check for suppression comment. Skip the check for diagnostics not
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -517,24 +517,8 @@
continue;
}
if (Tidy != nullptr) {
- std::string TidyDiag = Tidy->getCheckName(Diag.ID);
- if (!TidyDiag.empty()) {
- Diag.Name = std::move(TidyDiag);
+ if (!Diag.Name.empty()) {
Diag.Source = Diag::ClangTidy;
- // clang-tidy bakes the name into diagnostic messages. Strip it out.
- // It would be much nicer to make clang-tidy not do this.
- auto CleanMessage = [&](std::string &Msg) {
- StringRef Rest(Msg);
- if (Rest.consume_back("]") && Rest.consume_back(Diag.Name) &&
- Rest.consume_back(" ["))
- Msg.resize(Rest.size());
- };
- CleanMessage(Diag.Message);
- for (auto &Note : Diag.Notes)
- CleanMessage(Note.Message);
- for (auto &Fix : Diag.Fixes)
- CleanMessage(Fix.Message);
- continue;
}
}
}
@@ -624,6 +608,7 @@
LastDiagLoc.reset();
LastDiagOriginallyError = OriginallyError;
LastDiag->ID = Info.getID();
+ LastDiag->Name = tidy::ClangTidyContext::getDiagnosticName(Info);
fillNonLocationData(DiagLevel, Info, *LastDiag);
LastDiag->InsideMainFile = true;
// Put it at the start of the main file, for a lack of a better place.
@@ -734,6 +719,7 @@
LastDiag = Diag();
FillDiagBase(*LastDiag);
+ LastDiag->Name = tidy::ClangTidyContext::getDiagnosticName(Info);
LastDiagLoc.emplace(Info.getLocation(), Info.getSourceManager());
LastDiagOriginallyError = OriginallyError;
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -123,7 +123,10 @@
/// Returns the name of the clang-tidy check which produced this
/// diagnostic ID.
- std::string getCheckName(unsigned DiagnosticID) const;
+ static StringRef getDiagnosticName(const Diagnostic &Info,
+ SmallVectorImpl<char> &Buffer);
+
+ static std::string getDiagnosticName(const Diagnostic &Info);
/// Returns \c true if the check is enabled for the \c CurrentFile.
///
@@ -204,8 +207,6 @@
std::string CurrentBuildDirectory;
- llvm::DenseMap<unsigned, 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
@@ -30,6 +30,7 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Regex.h"
#include <tuple>
@@ -50,14 +51,6 @@
DiagnosticsEngine::Level Level, StringRef Message,
ArrayRef<CharSourceRange> Ranges,
DiagOrStoredDiag Info) override {
- // Remove check name from the message.
- // FIXME: Remove this once there's a better way to pass check names than
- // appending the check name to the message in ClangTidyContext::diag and
- // using getCustomDiagID.
- std::string CheckNameInMessage = " [" + Error.DiagnosticName + "]";
- if (Message.endswith(CheckNameInMessage))
- Message = Message.substr(0, Message.size() - CheckNameInMessage.size());
-
auto TidyMessage =
Loc.isValid()
? tooling::DiagnosticMessage(Message, Loc.getManager(), Loc)
@@ -165,23 +158,30 @@
ClangTidyContext::~ClangTidyContext() = default;
+static constexpr llvm::StringLiteral CheckPrefix("<Tidy>:");
+
DiagnosticBuilder ClangTidyContext::diag(
StringRef CheckName, SourceLocation Loc, StringRef Description,
DiagnosticIDs::Level Level /* = DiagnosticIDs::Warning*/) {
assert(Loc.isValid());
- unsigned ID = DiagEngine->getDiagnosticIDs()->getCustomDiagID(
- Level, (Description + " [" + CheckName + "]").str());
- CheckNamesByDiagnosticID.try_emplace(ID, CheckName);
- return DiagEngine->Report(Loc, ID);
+ unsigned ID =
+ DiagEngine->getDiagnosticIDs()->getCustomDiagID(Level, Description);
+ auto Res = DiagEngine->Report(Loc, ID);
+ // Repurpse the Flag value to store the check name. clang-tidy checks don't
+ // have command line flags so this field is safe ro use, but add a prefix just
+ // to be sure.
+ Res << AddFlagValue(SmallString<64>{CheckPrefix, CheckName});
+ return Res;
}
DiagnosticBuilder ClangTidyContext::diag(
StringRef CheckName, StringRef Description,
DiagnosticIDs::Level Level /* = DiagnosticIDs::Warning*/) {
- unsigned ID = DiagEngine->getDiagnosticIDs()->getCustomDiagID(
- Level, (Description + " [" + CheckName + "]").str());
- CheckNamesByDiagnosticID.try_emplace(ID, CheckName);
- return DiagEngine->Report(ID);
+ unsigned ID =
+ DiagEngine->getDiagnosticIDs()->getCustomDiagID(Level, Description);
+ auto Res = DiagEngine->Report(ID);
+ Res.addFlagValue(SmallString<64>({CheckPrefix, CheckName}));
+ return Res;
}
DiagnosticBuilder ClangTidyContext::configurationDiag(
@@ -246,18 +246,30 @@
return WarningAsErrorFilter->contains(CheckName);
}
-std::string ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
- std::string ClangWarningOption = std::string(
- DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(DiagnosticID));
- if (!ClangWarningOption.empty())
- return "clang-diagnostic-" + ClangWarningOption;
- llvm::DenseMap<unsigned, std::string>::const_iterator I =
- CheckNamesByDiagnosticID.find(DiagnosticID);
- if (I != CheckNamesByDiagnosticID.end())
- return I->second;
+StringRef ClangTidyContext::getDiagnosticName(const Diagnostic &Info,
+ SmallVectorImpl<char> &Buffer) {
+ auto CheckName = Info.getDiags()->getFlagValue();
+ if (!CheckName.empty() && CheckName.consume_front(CheckPrefix)) {
+ return CheckName;
+ }
+ StringRef ClangWarningOption =
+ DiagnosticIDs::getWarningOptionForDiag(Info.getID());
+ if (!ClangWarningOption.empty()) {
+ constexpr llvm::StringLiteral Prefix("clang-diagnostic-");
+ Buffer.resize_for_overwrite(Prefix.size() + ClangWarningOption.size());
+ memcpy(Buffer.data(), Prefix.data(), Prefix.size());
+ memcpy(Buffer.data() + Prefix.size(), ClangWarningOption.data(),
+ ClangWarningOption.size());
+ return {Buffer.begin(), Buffer.size()};
+ }
return "";
}
+std::string ClangTidyContext::getDiagnosticName(const Diagnostic &Info) {
+ SmallString<64> Buffer;
+ return getDiagnosticName(Info, Buffer).str();
+}
+
ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
ClangTidyContext &Ctx, DiagnosticsEngine *ExternalDiagEngine,
bool RemoveIncompatibleErrors)
@@ -290,7 +302,8 @@
}
static bool IsNOLINTFound(StringRef NolintDirectiveText, StringRef Line,
- unsigned DiagID, const ClangTidyContext &Context) {
+ StringRef CheckName,
+ const ClangTidyContext &Context) {
const size_t NolintIndex = Line.find(NolintDirectiveText);
if (NolintIndex == StringRef::npos)
return false;
@@ -305,7 +318,6 @@
Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
// Allow disabling all the checks with "*".
if (ChecksStr != "*") {
- std::string CheckName = Context.getCheckName(DiagID);
// Allow specifying a few check names, delimited with comma.
SmallVector<StringRef, 1> Checks;
ChecksStr.split(Checks, ',', -1, false);
@@ -325,7 +337,7 @@
}
static bool LineIsMarkedWithNOLINT(const SourceManager &SM, SourceLocation Loc,
- unsigned DiagID,
+ StringRef CheckName,
const ClangTidyContext &Context,
bool AllowIO) {
FileID File;
@@ -337,21 +349,22 @@
// Check if there's a NOLINT on this line.
StringRef RestOfLine = Buffer->substr(Offset).split('\n').first;
- if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context))
+ if (IsNOLINTFound("NOLINT", RestOfLine, CheckName, Context))
return true;
// Check if there's a NOLINTNEXTLINE on the previous line.
StringRef PrevLine =
Buffer->substr(0, Offset).rsplit('\n').first.rsplit('\n').second;
- return IsNOLINTFound("NOLINTNEXTLINE", PrevLine, DiagID, Context);
+ return IsNOLINTFound("NOLINTNEXTLINE", PrevLine, CheckName, Context);
}
static bool LineIsMarkedWithNOLINTinMacro(const SourceManager &SM,
- SourceLocation Loc, unsigned DiagID,
+ SourceLocation Loc,
+ StringRef CheckName,
const ClangTidyContext &Context,
bool AllowIO) {
while (true) {
- if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context, AllowIO))
+ if (LineIsMarkedWithNOLINT(SM, Loc, CheckName, Context, AllowIO))
return true;
if (!Loc.isMacroID())
return false;
@@ -366,12 +379,14 @@
bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
const Diagnostic &Info, ClangTidyContext &Context,
bool AllowIO) {
+ SmallString<64> Buffer;
return Info.getLocation().isValid() &&
DiagLevel != DiagnosticsEngine::Error &&
DiagLevel != DiagnosticsEngine::Fatal &&
- LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(),
- Info.getLocation(), Info.getID(),
- Context, AllowIO);
+ LineIsMarkedWithNOLINTinMacro(
+ Info.getSourceManager(), Info.getLocation(),
+ ClangTidyContext::getDiagnosticName(Info, Buffer), Context,
+ AllowIO);
}
} // namespace tidy
@@ -398,7 +413,9 @@
"A diagnostic note can only be appended to a message.");
} else {
finalizeLastError();
- std::string CheckName = Context.getCheckName(Info.getID());
+ SmallString<64> Buffer;
+ llvm::StringRef CheckName =
+ ClangTidyContext::getDiagnosticName(Info, Buffer);
if (CheckName.empty()) {
// This is a compiler diagnostic without a warning option. Assign check
// name based on its level.
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
