[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
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 , ClangTidyContext ,
- 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 getBuffer(const SourceManager , 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 = SM.getSLocEntry(File, );
+ 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(), )
+ :
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
nridge updated this revision to Diff 253446.
nridge added a comment.
Address last 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
@@ -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 , ClangTidyContext ,
- 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 getBuffer(const SourceManager , 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 = SM.getSLocEntry(File, );
+ 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(), )
+ : Cache->getRawBuffer();
+ if (!Buffer || CharDataInvalid)
+return llvm::None;
+ return
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:708-710 +return std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, +M1.Message) < + std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message); nridge wrote: > nridge wrote: > > sammccall wrote: > > > nridge wrote: > > > > njames93 wrote: > > > > > This looks like a clang-format artifact, there are several other > > > > > below. Could these be removed from this patch > > > > If you insist, I can move them to a separate patch. I don't want to > > > > leave it unmodified because the change will come back every time > > > > someone touches the file. > > > I don't personally care about this too much, but the changes are somewhat > > > distracting in review, blame etc. > > > > > > The policy we've mostly followed is to format changed lines only (git > > > clang-format, turn format-on-save off) and leave misformatted code alone > > > until it's next touched. > > > > > > (Not sure if LLVM offers any guidance either way, but that's what Google > > > does internally and IME it's been great) > > The issue I have with that is that turning format-on-save off means you > > inevitably end up doing a lot of manual formatting as you write the code. > > With format-on-save, you can just write the tokens for a statement not > > caring about the formatting, do a save, and have the statement be formatted > > before you start writing the next one. > I guess what we really want is "format on save for modified lines only". I > found a [VSCode > extension](https://marketplace.visualstudio.com/items?itemName=Gruntfuggly.format-modified) > which does that, I'll give that a try. What I do (apart from when i forget) is run git clang-format before creating the patch, Then just don't worry about formatting while I'm writing the code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75286/new/ https://reviews.llvm.org/D75286 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
To illustrate another workflow: I also got format-on-save turned on, but filter the irrelevant changes before committing by doing `git add -p` instead of adding all modifications. On Sat, Mar 28, 2020 at 9:17 PM Nathan Ridge via Phabricator < revi...@reviews.llvm.org> wrote: > nridge marked an inline comment as done. > nridge added inline comments. > > > > Comment at: > clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:708-710 > +return std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, > +M1.Message) < > + std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, > M2.Message); > > nridge wrote: > > sammccall wrote: > > > nridge wrote: > > > > njames93 wrote: > > > > > This looks like a clang-format artifact, there are several other > below. Could these be removed from this patch > > > > If you insist, I can move them to a separate patch. I don't want to > leave it unmodified because the change will come back every time someone > touches the file. > > > I don't personally care about this too much, but the changes are > somewhat distracting in review, blame etc. > > > > > > The policy we've mostly followed is to format changed lines only (git > clang-format, turn format-on-save off) and leave misformatted code alone > until it's next touched. > > > > > > (Not sure if LLVM offers any guidance either way, but that's what > Google does internally and IME it's been great) > > The issue I have with that is that turning format-on-save off means you > inevitably end up doing a lot of manual formatting as you write the code. > With format-on-save, you can just write the tokens for a statement not > caring about the formatting, do a save, and have the statement be formatted > before you start writing the next one. > I guess what we really want is "format on save for modified lines only". I > found a [VSCode extension]( > https://marketplace.visualstudio.com/items?itemName=Gruntfuggly.format-modified) > which does that, I'll give that a try. > > > Repository: > rG LLVM Github Monorepo > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D75286/new/ > > https://reviews.llvm.org/D75286 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:708-710 +return std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, +M1.Message) < + std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message); sammccall wrote: > nridge wrote: > > njames93 wrote: > > > This looks like a clang-format artifact, there are several other below. > > > Could these be removed from this patch > > If you insist, I can move them to a separate patch. I don't want to leave > > it unmodified because the change will come back every time someone touches > > the file. > I don't personally care about this too much, but the changes are somewhat > distracting in review, blame etc. > > The policy we've mostly followed is to format changed lines only (git > clang-format, turn format-on-save off) and leave misformatted code alone > until it's next touched. > > (Not sure if LLVM offers any guidance either way, but that's what Google does > internally and IME it's been great) The issue I have with that is that turning format-on-save off means you inevitably end up doing a lot of manual formatting as you write the code. With format-on-save, you can just write the tokens for a statement not caring about the formatting, do a save, and have the statement be formatted before you start writing the next one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75286/new/ https://reviews.llvm.org/D75286 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:708-710 +return std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, +M1.Message) < + std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message); nridge wrote: > sammccall wrote: > > nridge wrote: > > > njames93 wrote: > > > > This looks like a clang-format artifact, there are several other below. > > > > Could these be removed from this patch > > > If you insist, I can move them to a separate patch. I don't want to leave > > > it unmodified because the change will come back every time someone > > > touches the file. > > I don't personally care about this too much, but the changes are somewhat > > distracting in review, blame etc. > > > > The policy we've mostly followed is to format changed lines only (git > > clang-format, turn format-on-save off) and leave misformatted code alone > > until it's next touched. > > > > (Not sure if LLVM offers any guidance either way, but that's what Google > > does internally and IME it's been great) > The issue I have with that is that turning format-on-save off means you > inevitably end up doing a lot of manual formatting as you write the code. > With format-on-save, you can just write the tokens for a statement not caring > about the formatting, do a save, and have the statement be formatted before > you start writing the next one. I guess what we really want is "format on save for modified lines only". I found a [VSCode extension](https://marketplace.visualstudio.com/items?itemName=Gruntfuggly.format-modified) which does that, I'll give that a try. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75286/new/ https://reviews.llvm.org/D75286 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
sammccall added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:708-710 +return std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, +M1.Message) < + std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message); nridge wrote: > njames93 wrote: > > This looks like a clang-format artifact, there are several other below. > > Could these be removed from this patch > If you insist, I can move them to a separate patch. I don't want to leave it > unmodified because the change will come back every time someone touches the > file. I don't personally care about this too much, but the changes are somewhat distracting in review, blame etc. The policy we've mostly followed is to format changed lines only (git clang-format, turn format-on-save off) and leave misformatted code alone until it's next touched. (Not sure if LLVM offers any guidance either way, but that's what Google does internally and IME it's been great) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75286/new/ https://reviews.llvm.org/D75286 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:708-710 +return std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, +M1.Message) < + std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message); njames93 wrote: > This looks like a clang-format artifact, there are several other below. Could > these be removed from this patch If you insist, I can move them to a separate patch. I don't want to leave it unmodified because the change will come back every time someone touches the file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75286/new/ https://reviews.llvm.org/D75286 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
njames93 added inline comments.
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:708-710
+return std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName,
+M1.Message) <
+ std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName,
M2.Message);
This looks like a clang-format artifact, there are several other below. Could
these be removed from this patch
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:64
-MATCHER_P(FixMessage, Message, "") {
- return arg.Message == Message;
-}
+MATCHER_P(FixMessage, Message, "") { return arg.Message == Message; }
ditto
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:264-265
+ // Verify that we don't have "[check-name]" suffix in the
message.
+ WithFix(FixMessage(
+ "use a trailing return type for this function");
}
ditto
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:841-842
- 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'")));
ditto
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75286/new/
https://reviews.llvm.org/D75286
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Thanks a lot!
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:301
+bool AllowIO) {
+ // This is similar to the implementation of
SourceManager::getCharacterData(),
+ // but uses ContentCache::getRawBuffer() rather than getBuffer() if
nit: the peer function to reference is SourceManager::getBufferData
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75286/new/
https://reviews.llvm.org/D75286
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
nridge requested review of this revision. nridge added a comment. Thanks for the suggested simplification. As the change is not completely trivial, could you have one more look before landing? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75286/new/ https://reviews.llvm.org/D75286 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
nridge updated this revision to Diff 253005.
nridge added a comment.
Finish an incomplete variable extraction
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
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
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
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Sorry for taking a while to come back to this again. A bit chaotic right now :-)
Looks good, some further simplification possible.
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:298
+static const char *getCharacterData(const SourceManager , SourceLocation
Loc,
+bool *Invalid, bool AvoidIO) {
This seems more complicated than necessary - e.g. it replicates the "invalid"
handling from SourceManager::getCharacterData, when we just bail out in this
case. And keeping the raw char* interfaces seems unfortunate if we're going to
the trouble of reimplementing most of this anyway.
What about implementing `llvm::Optional getBuffer(FileID File, bool
AllowIO)`?
Where AllowIO determines whether you call getbuffer or getrawbuffer.
This is simpler than the current function, and the caller can be a bit more
expressive/simpler like:
```
unsigned Offset = SM.getFileOffset(Loc);
StringRef RestOfLine = Data.substr(Offset).split('\n').first;
StringRef PrevLine = Data.substr(0,
Offset).rsplit('\n').first.rsplit('\n').second;
```
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:220
const Diagnostic , ClangTidyContext
,
- bool CheckMacroExpansion = true);
+ bool AvoidIO = false);
nit: I'd use `AllowIO=true` to avoid the negative sense, up to you.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75286/new/
https://reviews.llvm.org/D75286
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
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.
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:215 /// 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 a valid FileID is passed as `FileFilter`, the function does not attempt +/// to read source files other than the one identified by the filter. A degree sammccall wrote: > I do wonder a how hard it would be to approach this more directly: > (conditionally )use some API for pulling code out of the SourceManager that > *won't* silently do IO. > ContentCache::getRawBuffer() or something? > > Up to you whether to look at this at all of course, this approach is better > than what we have now. I gave this a try in the updated patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75286/new/ https://reviews.llvm.org/D75286 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
sammccall added inline comments.
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:353
+SM.getFileID(Loc) != FileFilter) {
+ return false;
+}
why are we breaking the loop if we hit another file ID, rather than just not
checking it?
I think a macro token can be expanded into another macro (with a non-main file
ID) which in turn expands into the main-file - we'd want to reach the main file
in that case.
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:215
/// 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 a valid FileID is passed as `FileFilter`, the function does not attempt
+/// to read source files other than the one identified by the filter. A degree
I do wonder a how hard it would be to approach this more directly:
(conditionally )use some API for pulling code out of the SourceManager that
*won't* silently do IO.
ContentCache::getRawBuffer() or something?
Up to you whether to look at this at all of course, this approach is better
than what we have now.
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:288
// 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.
this part of the comment seems stale
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:278
+ #define BAD 8 / i
+ double f = BAD; // NOLINT
+ double g = [[8]] / i;
you may want a case with two levels of expansion
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75286/new/
https://reviews.llvm.org/D75286
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75286: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions
nridge created this revision.
nridge added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous,
MaskRay, ilya-biryukov.
Herald added a project: clang.
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
Repository:
rG LLVM Github Monorepo
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
@@ -274,7 +274,9 @@
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;
}
)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
@@ -290,9 +290,10 @@
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,
+ Info.getSourceManager().getMainFileID())) {
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,14 @@
/// 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 a valid FileID is passed as `FileFilter`, the function does not attempt
+/// to read source files other than the one identified by the filter. A degree
+/// of control over this is needed because in some use cases we cannot rely on
+/// files other than the one containing the diagnostic being mapped in the
+/// SourceManager.
bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
const Diagnostic , ClangTidyContext ,
- bool CheckMacroExpansion = true);
+ FileID FileFilter = FileID{});
/// 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
@@ -120,7 +120,6 @@
: tooling::Diagnostic(CheckName, DiagLevel, BuildDirectory),
IsWarningAsError(IsWarningAsError) {}
-
class ClangTidyContext::CachedGlobList {
public:
CachedGlobList(StringRef Globs) : Globs(Globs) {}
@@ -341,13 +340,18 @@
static bool LineIsMarkedWithNOLINTinMacro(const SourceManager ,
SourceLocation Loc, unsigned DiagID,
- const ClangTidyContext ) {
+ const ClangTidyContext ,
+ FileID FileFilter) {
while (true) {
if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context))
return true;
if (!Loc.isMacroID())
return false;
Loc = SM.getImmediateExpansionRange(Loc).getBegin();
+if (FileFilter.isValid() && Loc.isFileID() &&
+SM.getFileID(Loc) != FileFilter) {
+ return false;
+}
}
return false;
}
@@ -357,14 +361,13 @@
bool
