nridge updated this revision to Diff 248598.
nridge marked 4 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
@@ -285,14 +285,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,
+ /*AvoidIO=*/true)) {
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
@@ -212,15 +212,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 `AvoidIO` is true, 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 AvoidIO = false);
/// 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
@@ -123,7 +123,6 @@
: tooling::Diagnostic(CheckName, DiagLevel, BuildDirectory),
IsWarningAsError(IsWarningAsError) {}
-
class ClangTidyContext::CachedGlobList {
public:
CachedGlobList(StringRef Globs) : Globs(Globs) {}
@@ -296,11 +295,38 @@
return true;
}
+static const char *getCharacterData(const SourceManager &SM, SourceLocation Loc,
+ bool *Invalid, bool AvoidIO) {
+ if (!AvoidIO)
+ return SM.getCharacterData(Loc, Invalid);
+
+ // The rest is similar to the implementation of
+ // SourceManager::getCharacterData(), but uses ContentCache::getRawBuffer()
+ // rather than getBuffer() to avoid triggering file I/O if the file contents
+ // aren't already mapped.
+ std::pair<FileID, unsigned> LocInfo = SM.getDecomposedSpellingLoc(Loc);
+ bool CharDataInvalid = false;
+ const SrcMgr::SLocEntry &Entry =
+ SM.getSLocEntry(LocInfo.first, &CharDataInvalid);
+ if (CharDataInvalid || !Entry.isFile()) {
+ *Invalid = true;
+ return "<<<<INVALID BUFFER>>>>";
+ }
+ const llvm::MemoryBuffer *Buffer =
+ Entry.getFile().getContentCache()->getRawBuffer();
+ if (!Buffer) {
+ *Invalid = true;
+ return "<<<<NOT MAPPED>>>>";
+ }
+ return Buffer->getBufferStart() + (CharDataInvalid ? 0 : LocInfo.second);
+}
+
static bool LineIsMarkedWithNOLINT(const SourceManager &SM, SourceLocation Loc,
unsigned DiagID,
- const ClangTidyContext &Context) {
+ const ClangTidyContext &Context,
+ bool AvoidIO) {
bool Invalid;
- const char *CharacterData = SM.getCharacterData(Loc, &Invalid);
+ const char *CharacterData = getCharacterData(SM, Loc, &Invalid, AvoidIO);
if (Invalid)
return false;
@@ -313,8 +339,8 @@
return true;
// Check if there's a NOLINTNEXTLINE on the previous line.
- const char *BufBegin =
- SM.getCharacterData(SM.getLocForStartOfFile(SM.getFileID(Loc)), &Invalid);
+ const char *BufBegin = getCharacterData(
+ SM, SM.getLocForStartOfFile(SM.getFileID(Loc)), &Invalid, AvoidIO);
if (Invalid || P == BufBegin)
return false;
@@ -344,9 +370,10 @@
static bool LineIsMarkedWithNOLINTinMacro(const SourceManager &SM,
SourceLocation Loc, unsigned DiagID,
- const ClangTidyContext &Context) {
+ const ClangTidyContext &Context,
+ bool AvoidIO) {
while (true) {
- if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context))
+ if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context, AvoidIO))
return true;
if (!Loc.isMacroID())
return false;
@@ -360,14 +387,13 @@
bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
const Diagnostic &Info, ClangTidyContext &Context,
- bool CheckMacroExpansion) {
+ bool AvoidIO) {
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, AvoidIO);
}
} // namespace tidy
@@ -709,9 +735,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
