This revision was automatically updated to reflect the committed changes.
Closed by commit rGb9d9968f63ab: [clangd] Handle clang-tidy suppression
comments for diagnostics inside macro… (authored by nridge).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75286/new/
https://reviews.llvm.org/D75286
Files:
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
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -272,7 +272,11 @@
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());
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -310,14 +310,13 @@
// 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;
}
}
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -216,15 +216,12 @@
/// 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.
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -296,57 +296,54 @@
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 @@
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
