nridge updated this revision to Diff 253004.
nridge marked 2 inline comments as done.
nridge added a comment.
Address review comments
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
@@ -61,9 +61,7 @@
arg.Edits[0].range == Range && arg.Edits[0].newText == Replacement;
}
-MATCHER_P(FixMessage, Message, "") {
- return arg.Message == Message;
-}
+MATCHER_P(FixMessage, Message, "") { return arg.Message == Message; }
MATCHER_P(EqualToLSPDiag, LSPDiag,
"LSP diagnostic " + llvm::to_string(LSPDiag)) {
@@ -258,13 +256,13 @@
Diag(Test.range("macroarg"),
"multiple unsequenced modifications to 'y'"),
AllOf(
- Diag(Test.range("main"),
- "use a trailing return type for this function"),
- DiagSource(Diag::ClangTidy),
- DiagName("modernize-use-trailing-return-type"),
- // Verify that we don't have "[check-name]" suffix in the message.
- WithFix(FixMessage("use a trailing return type for this function")))
- ));
+ Diag(Test.range("main"),
+ "use a trailing return type for this function"),
+ DiagSource(Diag::ClangTidy),
+ DiagName("modernize-use-trailing-return-type"),
+ // Verify that we don't have "[check-name]" suffix in the message.
+ WithFix(FixMessage(
+ "use a trailing return type for this function")))));
}
TEST(DiagnosticTest, ClangTidySuppressionComment) {
@@ -274,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());
@@ -836,8 +838,9 @@
auto Index = buildIndexWithSymbol({});
TU.ExternalIndex = Index.get();
- EXPECT_THAT(TU.build().getDiagnostics(),
- ElementsAre(Diag(Test.range(), "use of undeclared identifier 'a'")));
+ EXPECT_THAT(
+ TU.build().getDiagnostics(),
+ ElementsAre(Diag(Test.range(), "use of undeclared identifier 'a'")));
}
TEST(DiagsInHeaders, DiagInsideHeader) {
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
@@ -124,7 +124,6 @@
: tooling::Diagnostic(CheckName, DiagLevel, BuildDirectory),
IsWarningAsError(IsWarningAsError) {}
-
class ClangTidyContext::CachedGlobList {
public:
CachedGlobList(StringRef Globs) : Globs(Globs) {}
@@ -297,57 +296,54 @@
return true;
}
+llvm::Optional<StringRef> getBuffer(const SourceManager &SM, FileID File,
+ bool AllowIO) {
+ // This is similar to the implementation of SourceManager::getCharacterData(),
+ // 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)
+ : Entry.getFile().getContentCache()->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;
@@ -361,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
@@ -710,9 +705,9 @@
const tooling::DiagnosticMessage &M1 = LHS.Message;
const tooling::DiagnosticMessage &M2 = RHS.Message;
- return
- std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, M1.Message) <
- std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message);
+ return std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName,
+ M1.Message) <
+ std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message);
}
};
struct EqualClangTidyError {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
