[clang-tools-extra] b9d9968 - [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
Nathan Ridge via cfe-commits
cfe-commits at lists.llvm.org
Sun Mar 29 12:20:27 PDT 2020
Author: Nathan Ridge
Date: 2020-03-29T15:19:13-04:00
New Revision: b9d9968f63ab8f24b300c69be11eadda3d405ac5
URL: https://github.com/llvm/llvm-project/commit/b9d9968f63ab8f24b300c69be11eadda3d405ac5
DIFF: https://github.com/llvm/llvm-project/commit/b9d9968f63ab8f24b300c69be11eadda3d405ac5.diff
LOG: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
Summary:
Not handling this was a side-effect of being overly cautious when trying
to avoid reading files for which clangd doesn't have the source mapped.
Fixes https://github.com/clangd/clangd/issues/266
Reviewers: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet,
usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D75286
Added:
Modified:
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 10d5bdb3b3af..d30552734679 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -296,57 +296,54 @@ static bool IsNOLINTFound(StringRef NolintDirectiveText, StringRef Line,
return true;
}
+llvm::Optional<StringRef> getBuffer(const SourceManager &SM, FileID File,
+ bool AllowIO) {
+ // This is similar to the implementation of SourceManager::getBufferData(),
+ // but uses ContentCache::getRawBuffer() rather than getBuffer() if
+ // AllowIO=false, to avoid triggering file I/O if the file contents aren't
+ // already mapped.
+ bool CharDataInvalid = false;
+ const SrcMgr::SLocEntry &Entry = SM.getSLocEntry(File, &CharDataInvalid);
+ if (CharDataInvalid || !Entry.isFile())
+ return llvm::None;
+ const SrcMgr::ContentCache *Cache = Entry.getFile().getContentCache();
+ const llvm::MemoryBuffer *Buffer =
+ AllowIO ? Cache->getBuffer(SM.getDiagnostics(), SM.getFileManager(),
+ SourceLocation(), &CharDataInvalid)
+ : Cache->getRawBuffer();
+ if (!Buffer || CharDataInvalid)
+ return llvm::None;
+ return Buffer->getBuffer();
+}
+
static bool LineIsMarkedWithNOLINT(const SourceManager &SM, SourceLocation Loc,
unsigned DiagID,
- const ClangTidyContext &Context) {
- bool Invalid;
- const char *CharacterData = SM.getCharacterData(Loc, &Invalid);
- if (Invalid)
+ const ClangTidyContext &Context,
+ bool AllowIO) {
+ FileID File;
+ unsigned Offset;
+ std::tie(File, Offset) = SM.getDecomposedSpellingLoc(Loc);
+ llvm::Optional<StringRef> Buffer = getBuffer(SM, File, AllowIO);
+ if (!Buffer)
return false;
// Check if there's a NOLINT on this line.
- const char *P = CharacterData;
- while (*P != '\0' && *P != '\r' && *P != '\n')
- ++P;
- StringRef RestOfLine(CharacterData, P - CharacterData + 1);
+ StringRef RestOfLine = Buffer->substr(Offset).split('\n').first;
if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context))
return true;
// Check if there's a NOLINTNEXTLINE on the previous line.
- const char *BufBegin =
- SM.getCharacterData(SM.getLocForStartOfFile(SM.getFileID(Loc)), &Invalid);
- if (Invalid || P == BufBegin)
- return false;
-
- // Scan backwards over the current line.
- P = CharacterData;
- while (P != BufBegin && *P != '\n')
- --P;
-
- // If we reached the begin of the file there is no line before it.
- if (P == BufBegin)
- return false;
-
- // Skip over the newline.
- --P;
- const char *LineEnd = P;
-
- // Now we're on the previous line. Skip to the beginning of it.
- while (P != BufBegin && *P != '\n')
- --P;
-
- RestOfLine = StringRef(P, LineEnd - P + 1);
- if (IsNOLINTFound("NOLINTNEXTLINE", RestOfLine, DiagID, Context))
- return true;
-
- return false;
+ StringRef PrevLine =
+ Buffer->substr(0, Offset).rsplit('\n').first.rsplit('\n').second;
+ return IsNOLINTFound("NOLINTNEXTLINE", PrevLine, DiagID, Context);
}
static bool LineIsMarkedWithNOLINTinMacro(const SourceManager &SM,
SourceLocation Loc, unsigned DiagID,
- const ClangTidyContext &Context) {
+ const ClangTidyContext &Context,
+ bool AllowIO) {
while (true) {
- if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context))
+ if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context, AllowIO))
return true;
if (!Loc.isMacroID())
return false;
@@ -360,14 +357,13 @@ namespace tidy {
bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
const Diagnostic &Info, ClangTidyContext &Context,
- bool CheckMacroExpansion) {
+ bool AllowIO) {
return Info.getLocation().isValid() &&
DiagLevel != DiagnosticsEngine::Error &&
DiagLevel != DiagnosticsEngine::Fatal &&
- (CheckMacroExpansion ? LineIsMarkedWithNOLINTinMacro
- : LineIsMarkedWithNOLINT)(Info.getSourceManager(),
- Info.getLocation(),
- Info.getID(), Context);
+ LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(),
+ Info.getLocation(), Info.getID(),
+ Context, AllowIO);
}
} // namespace tidy
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
index 18ce478805f8..2bb3296b150d 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -216,15 +216,12 @@ class ClangTidyContext {
/// This does not handle suppression of notes following a suppressed diagnostic;
/// that is left to the caller is it requires maintaining state in between calls
/// to this function.
-/// The `CheckMacroExpansion` parameter determines whether the function should
-/// handle the case where the diagnostic is inside a macro expansion. A degree
-/// of control over this is needed because handling this case can require
-/// examining source files other than the one in which the diagnostic is
-/// located, and in some use cases we cannot rely on such other files being
-/// mapped in the SourceMapper.
+/// If `AllowIO` is false, the function does not attempt to read source files
+/// from disk which are not already mapped into memory; such files are treated
+/// as not containing a suppression comment.
bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
const Diagnostic &Info, ClangTidyContext &Context,
- bool CheckMacroExpansion = true);
+ bool AllowIO = true);
/// A diagnostic consumer that turns each \c Diagnostic into a
/// \c SourceManager-independent \c ClangTidyError.
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 91c71789dbe0..1d6997f0b4d4 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -310,14 +310,13 @@ ParsedAST::build(llvm::StringRef Version,
// Check for suppression comment. Skip the check for diagnostics not
// in the main file, because we don't want that function to query the
// source buffer for preamble files. For the same reason, we ask
- // shouldSuppressDiagnostic not to follow macro expansions, since
- // those might take us into a preamble file as well.
+ // shouldSuppressDiagnostic to avoid I/O.
bool IsInsideMainFile =
Info.hasSourceManager() &&
isInsideMainFile(Info.getLocation(), Info.getSourceManager());
- if (IsInsideMainFile && tidy::shouldSuppressDiagnostic(
- DiagLevel, Info, *CTContext,
- /* CheckMacroExpansion = */ false)) {
+ if (IsInsideMainFile &&
+ tidy::shouldSuppressDiagnostic(DiagLevel, Info, *CTContext,
+ /*AllowIO=*/false)) {
return DiagnosticsEngine::Ignored;
}
}
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index d72f722921a8..00dd1f864813 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -272,7 +272,11 @@ TEST(DiagnosticTest, ClangTidySuppressionComment) {
double d = 8 / i; // NOLINT
// NOLINTNEXTLINE
double e = 8 / i;
- double f = [[8]] / i;
+ #define BAD 8 / i
+ double f = BAD; // NOLINT
+ double g = [[8]] / i;
+ #define BAD2 BAD
+ double h = BAD2; // NOLINT
}
)cpp");
TestTU TU = TestTU::withCode(Main.code());
More information about the cfe-commits
mailing list