[PATCH] D44142: [clangd] Revamp handling of diagnostics.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE327282: [clangd] Revamp handling of diagnostics. (authored
by ibiryukov, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D44142?vs=138004&id=138018#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44142
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Diagnostics.cpp
clangd/Diagnostics.h
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
test/clangd/diagnostics.test
test/clangd/execute-command.test
test/clangd/extra-flags.test
test/clangd/fixits.test
unittests/clangd/ClangdTests.cpp
unittests/clangd/ClangdUnitTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
unittests/clangd/XRefsTests.cpp
Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -46,9 +46,8 @@
private:
// Implement DiagnosticsConsumer.
- virtual void
- onDiagnosticsReady(PathRef File,
- Tagged> Diagnostics) override;
+ void onDiagnosticsReady(PathRef File,
+ Tagged> Diagnostics) override;
// Implement ProtocolCallbacks.
void onInitialize(InitializeParams &Params) override;
@@ -74,7 +73,7 @@
void onHover(TextDocumentPositionParams &Params) override;
void onChangeConfiguration(DidChangeConfigurationParams &Params) override;
- std::vector getFixIts(StringRef File, const clangd::Diagnostic &D);
+ std::vector getFixes(StringRef File, const clangd::Diagnostic &D);
JSONOutput &Out;
/// Used to indicate that the 'shutdown' request was received from the
@@ -87,8 +86,7 @@
bool IsDone = false;
std::mutex FixItsMutex;
- typedef std::map,
- LSPDiagnosticCompare>
+ typedef std::map, LSPDiagnosticCompare>
DiagnosticToReplacementMap;
/// Caches FixIts per file and diagnostics
llvm::StringMap FixItsMap;
Index: clangd/SourceCode.h
===
--- clangd/SourceCode.h
+++ clangd/SourceCode.h
@@ -30,6 +30,10 @@
/// Turn a SourceLocation into a [line, column] pair.
Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc);
+// Converts a half-open clang source range to an LSP range.
+// Note that clang also uses closed source ranges, which this can't handle!
+Range halfOpenToRange(const SourceManager &SM, CharSourceRange R);
+
} // namespace clangd
} // namespace clang
#endif
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -9,7 +9,9 @@
#include "ClangdUnit.h"
#include "Compiler.h"
+#include "Diagnostics.h"
#include "Logger.h"
+#include "SourceCode.h"
#include "Trace.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Frontend/CompilerInvocation.h"
@@ -81,22 +83,6 @@
std::vector TopLevelDecls;
};
-// Converts a half-open clang source range to an LSP range.
-// Note that clang also uses closed source ranges, which this can't handle!
-Range toRange(CharSourceRange R, const SourceManager &M) {
- // Clang is 1-based, LSP uses 0-based indexes.
- Position Begin;
- Begin.line = static_cast(M.getSpellingLineNumber(R.getBegin())) - 1;
- Begin.character =
- static_cast(M.getSpellingColumnNumber(R.getBegin())) - 1;
-
- Position End;
- End.line = static_cast(M.getSpellingLineNumber(R.getEnd())) - 1;
- End.character = static_cast(M.getSpellingColumnNumber(R.getEnd())) - 1;
-
- return {Begin, End};
-}
-
class InclusionLocationsCollector : public PPCallbacks {
public:
InclusionLocationsCollector(SourceManager &SourceMgr,
@@ -115,7 +101,7 @@
if (SourceMgr.isInMainFile(SR.getBegin())) {
// Only inclusion directives in the main file make sense. The user cannot
// select directives not in the main file.
- IncLocations.emplace_back(toRange(FilenameRange, SourceMgr),
+ IncLocations.emplace_back(halfOpenToRange(SourceMgr, FilenameRange),
File->tryGetRealPathName());
}
}
@@ -170,113 +156,6 @@
SourceManager *SourceMgr = nullptr;
};
-/// Convert from clang diagnostic level to LSP severity.
-static int getSeverity(DiagnosticsEngine::Level L) {
- switch (L) {
- case DiagnosticsEngine::Remark:
-return 4;
- case DiagnosticsEngine::Note:
-return 3;
- case DiagnosticsEngine::Warning:
-return 2;
- case DiagnosticsEngine::Fatal:
- case DiagnosticsEngine::Error:
-return 1;
- case DiagnosticsEngine::Ignored:
-return 0;
- }
- llvm_unreachable("Unknown diagnostic level!");
-}
-
-// Checks whether a location is within a half-open range.
-// Note that clang al
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
ilya-biryukov updated this revision to Diff 138004.
ilya-biryukov added a comment.
- Replace equality comparison ops with matchers, they were only used in tests.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44142
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Diagnostics.cpp
clangd/Diagnostics.h
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
test/clangd/diagnostics.test
test/clangd/execute-command.test
test/clangd/extra-flags.test
test/clangd/fixits.test
unittests/clangd/ClangdTests.cpp
unittests/clangd/ClangdUnitTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -41,8 +41,8 @@
using testing::UnorderedElementsAreArray;
class IgnoreDiagnostics : public DiagnosticsConsumer {
- void onDiagnosticsReady(
- PathRef File, Tagged> Diagnostics) override {}
+ void onDiagnosticsReady(PathRef File,
+ Tagged> Diagnostics) override {}
};
// FIXME: this is duplicated with FileIndexTests. Share it.
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,7 +21,7 @@
using ::testing::Pair;
using ::testing::Pointee;
-void ignoreUpdate(llvm::Optional>) {}
+void ignoreUpdate(llvm::Optional>) {}
void ignoreError(llvm::Error Err) {
handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
}
@@ -102,20 +102,20 @@
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero());
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
- [&](std::vector) { Ready.wait(); });
+ [&](std::vector) { Ready.wait(); });
S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "auto should have been cancelled by auto";
});
S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "no diags should not be called back";
});
S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
Ready.notify();
}
EXPECT_EQ(2, CallbackCount);
@@ -131,15 +131,15 @@
// FIXME: we could probably use timeouts lower than 1 second here.
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "auto should have been debounced and canceled";
});
std::this_thread::sleep_for(std::chrono::milliseconds(200));
S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
std::this_thread::sleep_for(std::chrono::seconds(2));
S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
}
EXPECT_EQ(2, CallbackCount);
}
@@ -190,8 +190,8 @@
{
WithContextValue WithNonce(NonceKey, ++Nonce);
S.update(File, Inputs, WantDiagnostics::Auto,
- [Nonce, &Mut, &TotalUpdates](
- llvm::Optional> Diags) {
+ [Nonce, &Mut,
+&TotalUpdates](llvm::Optional> Diags) {
EXPECT_THAT(Context::current().get(NonceKey),
Pointee(Nonce));
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -56,13 +56,13 @@
using ::testing::Contains;
using ::testing::Each;
using ::testing::ElementsAre;
+using ::testing::Field;
using ::testing::Not;
using ::testing::UnorderedElementsAre;
-using ::testing::Field;
class IgnoreDi
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Ship it! (but let's drop the extra operator== if possible?)
Comment at: clangd/ClangdLSPServer.cpp:329
+ {"title",
+ llvm::formatv("Apply FixIt {0}", GetFixitMessage(D.message))},
{"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND},
ilya-biryukov wrote:
> sammccall wrote:
> > ilya-biryukov wrote:
> > > sammccall wrote:
> > > > nit: while here, can we add a colon after FixIt (or if appropriate in
> > > > practice, just drop the prefix altogether?). My recollection is this is
> > > > a bit hard to parse.
> > > I added a colon for now, but happy to look into removing the prefix. The
> > > use-case that's slightly confusing is:
> > >
> > > ```use of undeclared identifier 'foo'. did you mean 'bar'?```
> > > Since the actual fix-it is at the end, it's might be a bit hard to
> > > understand.
> > Yeah. I'd still be inclined to drop the prefix:
> > - this case isn't ideal, but is also not terrible
> > - this is a "no separate note for fixit" case, where we can consider
> > synthesizing the message from the text edit rather than reusing the
> > diagnostic. That would work well at least in this case.
> >
> I still find a prefix useful in some cases, e.g. the `unresolved identifier
> foo. did you mean bar?`.
> Mostly for cases where a fix note uses the error message of the main
> diagnostic or when it's badly phrased (haven't really seen those, but I would
> be surprised if there are none).
that's... exactly the case I was replying to :)
I don't think it's common or confusing enough, and there are other fixes when
using the main diagnostic. But this isn't a big deal; let's leave it for now.
Comment at: clangd/Protocol.h:166
};
+inline bool operator==(const TextEdit &LHS, const TextEdit &RHS) {
+ return std::tie(LHS.range, LHS.newText) == std::tie(RHS.range, RHS.newText);
The TextEdit/Diagnostic == operators seem odd - what do we need these for?
Comment at: unittests/clangd/ClangdUnitTests.cpp:70
+class WithFixesMatcher : public testing::MatcherInterface {
+public:
ilya-biryukov wrote:
> sammccall wrote:
> > Ah, and now that you've written this... ;-)
> >
> > For the specific case of "field X matches matcher M" you can use
> > ::testing::Field (you'd keep WithNote, but make it call Field)
> >
> > Messages are good as long as you pass the optional field_name param.
> Was a useful exercise anyway.
>
> Couldn't find the optional 'field_name' param, though, am I missing something?
You're not, sorry - it was added recently to google's copy and hasn't been
pushed to upstream gmock yet. So for now we have slightly bad error messages
(or have the full MatcherInterface implementation)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44142
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:329
+ {"title",
+ llvm::formatv("Apply FixIt {0}", GetFixitMessage(D.message))},
{"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND},
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > nit: while here, can we add a colon after FixIt (or if appropriate in
> > > practice, just drop the prefix altogether?). My recollection is this is a
> > > bit hard to parse.
> > I added a colon for now, but happy to look into removing the prefix. The
> > use-case that's slightly confusing is:
> >
> > ```use of undeclared identifier 'foo'. did you mean 'bar'?```
> > Since the actual fix-it is at the end, it's might be a bit hard to
> > understand.
> Yeah. I'd still be inclined to drop the prefix:
> - this case isn't ideal, but is also not terrible
> - this is a "no separate note for fixit" case, where we can consider
> synthesizing the message from the text edit rather than reusing the
> diagnostic. That would work well at least in this case.
>
I still find a prefix useful in some cases, e.g. the `unresolved identifier
foo. did you mean bar?`.
Mostly for cases where a fix note uses the error message of the main diagnostic
or when it's badly phrased (haven't really seen those, but I would be surprised
if there are none).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44142
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
ilya-biryukov added inline comments.
Comment at: clangd/Diagnostics.h:35
+ DiagnosticsEngine::Level Severity;
+ llvm::SmallVector FixIts;
+ // Since File is only descriptive, we store a separate flag to distinguish
sammccall wrote:
> sammccall wrote:
> > As discussed offline - I think fixits should probably be a different struct
> > hanging off the main diagnostic, with a name. Following clang's example
> > seems... less than clear here.
> >
> > They could also be modeled as notes with an optional TextEdit - this seems
> > sligthly less clear but more faithful to clang internals*.
> > Either way, it should be clear that they're only allowed in *one* of these
> > locations - having notes and main diags be distinct types would help.
> >
> > I think this probably affects how we should expose them through LSP - they
> > should be named code actions attached to the original diagnostic.
> > Maybe they should also be separate diagnostics, but only if they contribute
> > a unique opportunity to the user e.g. they have a non-empty range that
> > isn't contained within the diagnostic's range.
> > This patch seems like a reasonable place to do that, but also OK if you
> > want to defer.
> Nit: can we call these "Fix"es, which is a noun (at least in common usage?)
>
> Clang calls them FixItHint, but I think Fix is no less clear (and shorter)
We now provide a separate `Fix` class, but we still don't return it to LSP,
keeping the behaviour as is (i.e. mapping LSP ranges to code actions on the
server side).
The fixes are not reported as separate diagnostics, they are only shown as code
actions when users hovers over the diagnostic.
Comment at: clangd/Diagnostics.h:57
+
+/// Conventional conversion to LSP diagnostics. Formats the error message of
+/// each diagnostic to include all its notes. Notes inside main file are also
sammccall wrote:
> I'm not sure what "conventional" means here?
Removed conventional from the comment. Other parts should make sense.
Comment at: unittests/clangd/ClangdUnitTests.cpp:70
+class WithFixesMatcher : public testing::MatcherInterface {
+public:
sammccall wrote:
> Ah, and now that you've written this... ;-)
>
> For the specific case of "field X matches matcher M" you can use
> ::testing::Field (you'd keep WithNote, but make it call Field)
>
> Messages are good as long as you pass the optional field_name param.
Was a useful exercise anyway.
Couldn't find the optional 'field_name' param, though, am I missing something?
Comment at: unittests/clangd/ClangdUnitTests.cpp:149
+/// Matches diagnostic that has exactly one fix with the same range and message
+/// as the diagnostic itself.
+testing::Matcher
sammccall wrote:
> this "same range and message as the diagnostic itself" is an important
> property of the code being tested, and it's hidden here in a comment of a
> helper function.
> I'd probably prefer avoiding this helper altogether (spelling the string
> twice in the test) so the test behavior/change is obvious when changing the
> code.
> Failing that, DiagWithEqualFix or something?
I agree that, but spelling this in the code is so much noise. Renaming to
`DiagWithEqualFix` makes total sense, did that.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44142
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
ilya-biryukov updated this revision to Diff 137987.
ilya-biryukov marked 14 inline comments as done.
ilya-biryukov added a comment.
- Addressed review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44142
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Diagnostics.cpp
clangd/Diagnostics.h
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
test/clangd/diagnostics.test
test/clangd/execute-command.test
test/clangd/extra-flags.test
test/clangd/fixits.test
unittests/clangd/ClangdTests.cpp
unittests/clangd/ClangdUnitTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -41,8 +41,8 @@
using testing::UnorderedElementsAreArray;
class IgnoreDiagnostics : public DiagnosticsConsumer {
- void onDiagnosticsReady(
- PathRef File, Tagged> Diagnostics) override {}
+ void onDiagnosticsReady(PathRef File,
+ Tagged> Diagnostics) override {}
};
// FIXME: this is duplicated with FileIndexTests. Share it.
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,7 +21,7 @@
using ::testing::Pair;
using ::testing::Pointee;
-void ignoreUpdate(llvm::Optional>) {}
+void ignoreUpdate(llvm::Optional>) {}
void ignoreError(llvm::Error Err) {
handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
}
@@ -102,20 +102,20 @@
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero());
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
- [&](std::vector) { Ready.wait(); });
+ [&](std::vector) { Ready.wait(); });
S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "auto should have been cancelled by auto";
});
S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "no diags should not be called back";
});
S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
Ready.notify();
}
EXPECT_EQ(2, CallbackCount);
@@ -131,15 +131,15 @@
// FIXME: we could probably use timeouts lower than 1 second here.
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "auto should have been debounced and canceled";
});
std::this_thread::sleep_for(std::chrono::milliseconds(200));
S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
std::this_thread::sleep_for(std::chrono::seconds(2));
S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
}
EXPECT_EQ(2, CallbackCount);
}
@@ -190,8 +190,8 @@
{
WithContextValue WithNonce(NonceKey, ++Nonce);
S.update(File, Inputs, WantDiagnostics::Auto,
- [Nonce, &Mut, &TotalUpdates](
- llvm::Optional> Diags) {
+ [Nonce, &Mut,
+&TotalUpdates](llvm::Optional> Diags) {
EXPECT_THAT(Context::current().get(NonceKey),
Pointee(Nonce));
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -56,13 +56,13 @@
using ::testing::Contains;
using ::testing::Each;
using ::testing::ElementsAre;
+using ::testing::Field;
using ::testing::Not;
using ::testing::UnorderedElementsAre;
-using ::testing::Field;
class IgnoreDiag
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
ilya-biryukov updated this revision to Diff 137985.
ilya-biryukov added a comment.
- Added unit test for toLSPDiags
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44142
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Diagnostics.cpp
clangd/Diagnostics.h
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
test/clangd/diagnostics.test
test/clangd/execute-command.test
test/clangd/extra-flags.test
test/clangd/fixits.test
unittests/clangd/ClangdTests.cpp
unittests/clangd/ClangdUnitTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -41,8 +41,8 @@
using testing::UnorderedElementsAreArray;
class IgnoreDiagnostics : public DiagnosticsConsumer {
- void onDiagnosticsReady(
- PathRef File, Tagged> Diagnostics) override {}
+ void onDiagnosticsReady(PathRef File,
+ Tagged> Diagnostics) override {}
};
// FIXME: this is duplicated with FileIndexTests. Share it.
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,7 +21,7 @@
using ::testing::Pair;
using ::testing::Pointee;
-void ignoreUpdate(llvm::Optional>) {}
+void ignoreUpdate(llvm::Optional>) {}
void ignoreError(llvm::Error Err) {
handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
}
@@ -102,20 +102,20 @@
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero());
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
- [&](std::vector) { Ready.wait(); });
+ [&](std::vector) { Ready.wait(); });
S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "auto should have been cancelled by auto";
});
S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "no diags should not be called back";
});
S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
Ready.notify();
}
EXPECT_EQ(2, CallbackCount);
@@ -131,15 +131,15 @@
// FIXME: we could probably use timeouts lower than 1 second here.
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "auto should have been debounced and canceled";
});
std::this_thread::sleep_for(std::chrono::milliseconds(200));
S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
std::this_thread::sleep_for(std::chrono::seconds(2));
S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
}
EXPECT_EQ(2, CallbackCount);
}
@@ -190,8 +190,8 @@
{
WithContextValue WithNonce(NonceKey, ++Nonce);
S.update(File, Inputs, WantDiagnostics::Auto,
- [Nonce, &Mut, &TotalUpdates](
- llvm::Optional> Diags) {
+ [Nonce, &Mut,
+&TotalUpdates](llvm::Optional> Diags) {
EXPECT_THAT(Context::current().get(NonceKey),
Pointee(Nonce));
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -56,13 +56,13 @@
using ::testing::Contains;
using ::testing::Each;
using ::testing::ElementsAre;
+using ::testing::Field;
using ::testing::Not;
using ::testing::UnorderedElementsAre;
-using ::testing::Field;
class IgnoreDiagnostics : public DiagnosticsConsumer {
- vo
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
sammccall added a comment.
Sorry, last comments were concurrent with the latest snapshot.
Except where noted, the old comments still apply I think.
Comment at: clangd/ClangdLSPServer.cpp:317
for (Diagnostic &D : Params.context.diagnostics) {
-auto Edits = getFixIts(Params.textDocument.uri.file(), D);
-if (!Edits.empty()) {
+auto Fixes = getFixIts(Params.textDocument.uri.file(), D);
+for (auto &F : Fixes) {
inline this into the for loop?
Comment at: clangd/ClangdLSPServer.h:76
- std::vector getFixIts(StringRef File, const clangd::Diagnostic &D);
+ std::vector getFixIts(StringRef File, const clangd::Diagnostic &D);
getFixes?
Comment at: clangd/Diagnostics.h:35
+ DiagnosticsEngine::Level Severity;
+ llvm::SmallVector FixIts;
+ // Since File is only descriptive, we store a separate flag to distinguish
sammccall wrote:
> sammccall wrote:
> > As discussed offline - I think fixits should probably be a different struct
> > hanging off the main diagnostic, with a name. Following clang's example
> > seems... less than clear here.
> >
> > They could also be modeled as notes with an optional TextEdit - this seems
> > sligthly less clear but more faithful to clang internals*.
> > Either way, it should be clear that they're only allowed in *one* of these
> > locations - having notes and main diags be distinct types would help.
> >
> > I think this probably affects how we should expose them through LSP - they
> > should be named code actions attached to the original diagnostic.
> > Maybe they should also be separate diagnostics, but only if they contribute
> > a unique opportunity to the user e.g. they have a non-empty range that
> > isn't contained within the diagnostic's range.
> > This patch seems like a reasonable place to do that, but also OK if you
> > want to defer.
> Nit: can we call these "Fix"es, which is a noun (at least in common usage?)
>
> Clang calls them FixItHint, but I think Fix is no less clear (and shorter)
This is done, and looks good to me.
Comment at: clangd/Diagnostics.h:40
+
+struct PersistentDiag {
+ /// Main diagnostic.
sammccall wrote:
> I don't understand what "persistent" means in this context.
> This does seem to be an is-a relationship rather than has-a, so I'd find
> DiagBase/Diag + inheritance clearer.
>
> But with composition, this one seems rike it should be "Diag" and the type of
> Main could be DiagDetails or so?
(this happened already, yay!)
Comment at: clangd/Diagnostics.h:47
+
+/// Represents a note for the diagnostic. Severity of this Diagnostic can only
+/// be 'note' or 'remark'.
nit: severity of notes can...
Comment at: clangd/Diagnostics.h:51
+
+/// A non-note diagnostic with Notes and fix-its.
+struct Diag : DiagBase {
Seems a bit confusing to use "diagnostic" to mean something importantly
different than "diag", and to define top-level diagnostic in terms of notes.
Maybe just "A top-level diagnostic that may have Notes and Fixes"?
Comment at: clangd/Diagnostics.h:54
+ std::vector Notes;
+ std::vector Fixes;
+};
I think these deserve some comments:
- fixes are *alternative* fixes for this diagnostic, one should be chosen
- notes elaborate on the problem, usually pointing to a related piece of code
Comment at: clangd/Diagnostics.h:57
+
+/// Conventional conversion to LSP diagnostics. Formats the error message of
+/// each diagnostic to include all its notes. Notes inside main file are also
I'm not sure what "conventional" means here?
Comment at: unittests/clangd/ClangdUnitTests.cpp:24
+
+void PrintTo(const DiagBase &D, std::ostream *O) {
+ llvm::raw_os_ostream OS(*O);
can you make these operator<< and move to the main code? This seems a fine
general-purpose debug representation (could be tweaked, but also later).
That avoids the ODR issue, and makes casual logging easier.
Comment at: unittests/clangd/ClangdUnitTests.cpp:70
+class WithFixesMatcher : public testing::MatcherInterface {
+public:
Ah, and now that you've written this... ;-)
For the specific case of "field X matches matcher M" you can use
::testing::Field (you'd keep WithNote, but make it call Field)
Messages are good as long as you pass the optional field_name param.
Comment at: unittests/clangd/ClangdUnitTests.cpp:149
+/// Matches diagnostic that has exactly one fix with the same range and message
+/// as the diagnostic itself.
+testing::Matcher
this "same range and message as the diagnostic itself" is an important property
of the code being tested, and it's hidden here in a comment of a helper
function.
I'd probably prefer avoiding this helper altogether (spelling
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
sammccall added a comment.
Not sure if you're waiting on comments from me: the changes look good.
As discussed I still think Fix should be a separate thing with a name because
that's how editors treat it, and this is the right layer to decide how to do
that mapping.
Comment at: clangd/ClangdLSPServer.cpp:329
+ {"title",
+ llvm::formatv("Apply FixIt {0}", GetFixitMessage(D.message))},
{"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND},
ilya-biryukov wrote:
> sammccall wrote:
> > nit: while here, can we add a colon after FixIt (or if appropriate in
> > practice, just drop the prefix altogether?). My recollection is this is a
> > bit hard to parse.
> I added a colon for now, but happy to look into removing the prefix. The
> use-case that's slightly confusing is:
>
> ```use of undeclared identifier 'foo'. did you mean 'bar'?```
> Since the actual fix-it is at the end, it's might be a bit hard to understand.
Yeah. I'd still be inclined to drop the prefix:
- this case isn't ideal, but is also not terrible
- this is a "no separate note for fixit" case, where we can consider
synthesizing the message from the text edit rather than reusing the diagnostic.
That would work well at least in this case.
Comment at: clangd/ClangdLSPServer.cpp:329
+ {"title",
+ llvm::formatv("Apply FixIt: {0}", GetFixitMessage(D.message))},
{"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND},
nit: fixit --> fix in prefix and in lambda
(And drop the caps in the prefix I think, as this is now a regular english noun
rather than a pseudo-product-name)
Comment at: clangd/Diagnostics.h:40
+
+struct PersistentDiag {
+ /// Main diagnostic.
I don't understand what "persistent" means in this context.
This does seem to be an is-a relationship rather than has-a, so I'd find
DiagBase/Diag + inheritance clearer.
But with composition, this one seems rike it should be "Diag" and the type of
Main could be DiagDetails or so?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44142
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
ilya-biryukov updated this revision to Diff 137977.
ilya-biryukov marked 8 inline comments as done.
ilya-biryukov added a comment.
Addressed review comments.
The only thing that's left on my todo-list is a test for toLSPDiags().
Everything else should be ready.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44142
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Diagnostics.cpp
clangd/Diagnostics.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
test/clangd/diagnostics.test
test/clangd/execute-command.test
test/clangd/extra-flags.test
test/clangd/fixits.test
unittests/clangd/ClangdTests.cpp
unittests/clangd/ClangdUnitTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -41,8 +41,8 @@
using testing::UnorderedElementsAreArray;
class IgnoreDiagnostics : public DiagnosticsConsumer {
- void onDiagnosticsReady(
- PathRef File, Tagged> Diagnostics) override {}
+ void onDiagnosticsReady(PathRef File,
+ Tagged> Diagnostics) override {}
};
// FIXME: this is duplicated with FileIndexTests. Share it.
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,7 +21,7 @@
using ::testing::Pair;
using ::testing::Pointee;
-void ignoreUpdate(llvm::Optional>) {}
+void ignoreUpdate(llvm::Optional>) {}
void ignoreError(llvm::Error Err) {
handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
}
@@ -102,20 +102,20 @@
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero());
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
- [&](std::vector) { Ready.wait(); });
+ [&](std::vector) { Ready.wait(); });
S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "auto should have been cancelled by auto";
});
S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "no diags should not be called back";
});
S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
Ready.notify();
}
EXPECT_EQ(2, CallbackCount);
@@ -131,15 +131,15 @@
// FIXME: we could probably use timeouts lower than 1 second here.
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "auto should have been debounced and canceled";
});
std::this_thread::sleep_for(std::chrono::milliseconds(200));
S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
std::this_thread::sleep_for(std::chrono::seconds(2));
S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
}
EXPECT_EQ(2, CallbackCount);
}
@@ -190,8 +190,8 @@
{
WithContextValue WithNonce(NonceKey, ++Nonce);
S.update(File, Inputs, WantDiagnostics::Auto,
- [Nonce, &Mut, &TotalUpdates](
- llvm::Optional> Diags) {
+ [Nonce, &Mut,
+&TotalUpdates](llvm::Optional> Diags) {
EXPECT_THAT(Context::current().get(NonceKey),
Pointee(Nonce));
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -56,13 +56,13 @@
using ::testing::Contains;
using ::testing::Each;
using ::testing::ElementsAre;
+using ::testing::Field;
using ::testing::Not;
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
ilya-biryukov updated this revision to Diff 137589.
ilya-biryukov marked 9 inline comments as done.
ilya-biryukov added a comment.
This is not final, there are still unadressed comments.
- Significantly simplified the implementation by removing the DiagList, etc.
- Addressed some of the rest of the comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44142
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Diagnostics.cpp
clangd/Diagnostics.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
test/clangd/diagnostics.test
test/clangd/execute-command.test
test/clangd/extra-flags.test
test/clangd/fixits.test
unittests/clangd/ClangdTests.cpp
unittests/clangd/ClangdUnitTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -42,7 +42,7 @@
class IgnoreDiagnostics : public DiagnosticsConsumer {
void onDiagnosticsReady(
- PathRef File, Tagged> Diagnostics) override {}
+ PathRef File, Tagged> Diagnostics) override {}
};
// FIXME: this is duplicated with FileIndexTests. Share it.
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,7 +21,7 @@
using ::testing::Pair;
using ::testing::Pointee;
-void ignoreUpdate(llvm::Optional>) {}
+void ignoreUpdate(llvm::Optional>) {}
void ignoreError(llvm::Error Err) {
handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
}
@@ -102,20 +102,20 @@
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero());
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
- [&](std::vector) { Ready.wait(); });
+ [&](std::vector) { Ready.wait(); });
S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "auto should have been cancelled by auto";
});
S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "no diags should not be called back";
});
S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
Ready.notify();
}
EXPECT_EQ(2, CallbackCount);
@@ -131,15 +131,15 @@
// FIXME: we could probably use timeouts lower than 1 second here.
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
+ [&](std::vector Diags) {
ADD_FAILURE() << "auto should have been debounced and canceled";
});
std::this_thread::sleep_for(std::chrono::milliseconds(200));
S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
std::this_thread::sleep_for(std::chrono::seconds(2));
S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](std::vector Diags) { ++CallbackCount; });
}
EXPECT_EQ(2, CallbackCount);
}
@@ -191,7 +191,7 @@
WithContextValue WithNonce(NonceKey, ++Nonce);
S.update(File, Inputs, WantDiagnostics::Auto,
[Nonce, &Mut, &TotalUpdates](
- llvm::Optional> Diags) {
+ llvm::Optional> Diags) {
EXPECT_THAT(Context::current().get(NonceKey),
Pointee(Nonce));
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -56,13 +56,13 @@
using ::testing::Contains;
using ::testing::Each;
using ::testing::ElementsAre;
+using ::testing::Field;
using ::testing::Not;
using ::testing::UnorderedElementsAre;
-using ::testing::Field;
class IgnoreDiagnostics : public
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:329
+ {"title",
+ llvm::formatv("Apply FixIt {0}", GetFixitMessage(D.message))},
{"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND},
sammccall wrote:
> nit: while here, can we add a colon after FixIt (or if appropriate in
> practice, just drop the prefix altogether?). My recollection is this is a bit
> hard to parse.
I added a colon for now, but happy to look into removing the prefix. The
use-case that's slightly confusing is:
```use of undeclared identifier 'foo'. did you mean 'bar'?```
Since the actual fix-it is at the end, it's might be a bit hard to understand.
Comment at: clangd/Diagnostics.cpp:200
+
+ DiagWithFixIts Main = PreBuild(D.main());
+ Main.Diag.message = presentMainMessage(D);
sammccall wrote:
> (nit: if the goal with the callback function is to avoid allocations,
> shouldn't we reuse the DiagWithFixIts?)
Can we actually avoid allocating some of the strings there?
We're `std::move`ing the result into the callback, so we don't seem to waste
memory too.
Comment at: clangd/Diagnostics.h:58
+/// by fields of PersistentDiag.
+class DiagList {
+public:
sammccall wrote:
> This is cool :) but really looks like premature optimization.
> Particularly, if the diagnostics were a self-contained value type, this could
> just be vector I think. And if we care about copying strings (I'm not
> sure we do), being lifetime-scoped to the ASTcontext is probably OK.
>
> (rawDiags() has potential uses but having callers explicitly traverse is
> going to be clearer as your comment suggests)
Using a straight-forward representation for now.
Comment at: clangd/Diagnostics.h:123
+/// still be included as part of their main diagnostic's message.
+void toLSPDiags(const DiagList &Diags,
+llvm::function_ref OutFn);
sammccall wrote:
> I don't think this overload pays for itself - making callers write the outer
> loop would be a little clearer, I think.
>
> (I'd consider having that fn return a smallvector instead of taking a
> callback, but don't feel strongly)
Still using a callback, but happy to return a `SmallVector` instead. It seems
the pattern for using `SmallVector` in llvm is passing it in as an output
parameter (so that the users might decide themselves what's the count of items
on the stack), which is slightly ugly.
It might be fine to return `SmallVector` here though, this function can
probably make assumptions about the number of notes in a diagnostic.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44142
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
sammccall added a comment.
Behavior looks good. I think we can do a bit better with (most) fixits - your
call on whether it makes sense to do here.
As discussed i'd simplify the diagnostic containers until we know there's a
strong need.
Comment at: clangd/ClangdLSPServer.cpp:329
+ {"title",
+ llvm::formatv("Apply FixIt {0}", GetFixitMessage(D.message))},
{"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND},
nit: while here, can we add a colon after FixIt (or if appropriate in practice,
just drop the prefix altogether?). My recollection is this is a bit hard to
parse.
Comment at: clangd/Diagnostics.cpp:85
+
+bool isInsdieMainFile(const clang::Diagnostic &D) {
+ if (!D.hasSourceManager())
inside
Comment at: clangd/Diagnostics.cpp:125
+
+void printDiag(llvm::raw_string_ostream &OS, const PersistentDiag &D) {
+ if (D.InsideMainFile) {
mind pasting a small example as a function comment?
Logic looks good, but I had to reconstruct the message in my head.
Comment at: clangd/Diagnostics.cpp:147
+
+std::string presentMainMessage(const DiagWithNotes &D) {
+ std::string Result;
or just mainMessage (this is another verbs-for-pure-functions case where the
style recommendation seems to hurt readability to me, but up to you)
Comment at: clangd/Diagnostics.cpp:192
+llvm::function_ref OutFn) {
+ auto PreBuild = [](const PersistentDiag &D) {
+DiagWithFixIts Res;
PreBuild could be FillBasicFields or something? found this name confusing.
Comment at: clangd/Diagnostics.cpp:200
+
+ DiagWithFixIts Main = PreBuild(D.main());
+ Main.Diag.message = presentMainMessage(D);
(nit: if the goal with the callback function is to avoid allocations, shouldn't
we reuse the DiagWithFixIts?)
Comment at: clangd/Diagnostics.cpp:322
+ addToOutput(D);
+ }
+ LastDiagAndNotes.clear();
do you need else log here? (and just log the main diag)
Comment at: clangd/Diagnostics.h:28
+/// DiagList.
+struct PersistentDiag {
+ llvm::StringRef Message;
ilya-biryukov wrote:
> This could probably use a better name
I found it a bit confusing that this represents both "top-level" diagnostics
and notes. It obscures the nature of the hierarchy a bit: there *are* a fixed
number of levels with different "kinds", but the kinds are similar enough to
share a type.
I'd actually consider using inheritance here to model the relationships more
clearly. (Yes, gross, I know)
Like:
struct DiagBase { Message, File, Range, InMainFile }
struct Diag : DiagBase { FixIts, Notes, Severity }
struct Note : DiagBase {} // or leave this one out until needed
(I think we came to the conclusion that different severity of notes isn't
interesting and potentially just confusing)
Comment at: clangd/Diagnostics.h:35
+ DiagnosticsEngine::Level Severity;
+ llvm::SmallVector FixIts;
+ // Since File is only descriptive, we store a separate flag to distinguish
As discussed offline - I think fixits should probably be a different struct
hanging off the main diagnostic, with a name. Following clang's example
seems... less than clear here.
They could also be modeled as notes with an optional TextEdit - this seems
sligthly less clear but more faithful to clang internals*.
Either way, it should be clear that they're only allowed in *one* of these
locations - having notes and main diags be distinct types would help.
I think this probably affects how we should expose them through LSP - they
should be named code actions attached to the original diagnostic.
Maybe they should also be separate diagnostics, but only if they contribute a
unique opportunity to the user e.g. they have a non-empty range that isn't
contained within the diagnostic's range.
This patch seems like a reasonable place to do that, but also OK if you want to
defer.
Comment at: clangd/Diagnostics.h:35
+ DiagnosticsEngine::Level Severity;
+ llvm::SmallVector FixIts;
+ // Since File is only descriptive, we store a separate flag to distinguish
sammccall wrote:
> As discussed offline - I think fixits should probably be a different struct
> hanging off the main diagnostic, with a name. Following clang's example
> seems... less than clear here.
>
> They could also be modeled as notes with an optional TextEdit - this seems
> sligthly less clear but more faithful to clang internals*.
> Either way, it should be clear that they're only allowed in *one* of these
> locations - having notes and main diags be distinct types would help.
>
> I think this probably affects how we should expose them through LSP - they
> should be named code actions attached to the original diagnostic.
> Maybe they
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
ilya-biryukov added inline comments.
Comment at: clangd/Diagnostics.h:28
+/// DiagList.
+struct PersistentDiag {
+ llvm::StringRef Message;
This could probably use a better name
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44142
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
ilya-biryukov added a comment. This revision is still missing tests, but I wanted to get feedback for the overall design and the new diagnostic messages. So sending out for review now. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44142 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44142: [clangd] Revamp handling of diagnostics.
ilya-biryukov created this revision.
ilya-biryukov added reviewers: hokein, ioeric, sammccall.
Herald added subscribers: jkorous-apple, mgorny, klimek.
The new implementation attaches notes to diagnostic message and shows
the original diagnostics in the message of the note.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44142
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/Diagnostics.cpp
clangd/Diagnostics.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
test/clangd/diagnostics.test
test/clangd/execute-command.test
test/clangd/extra-flags.test
test/clangd/fixits.test
unittests/clangd/ClangdTests.cpp
unittests/clangd/ClangdUnitTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/TUSchedulerTests.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -41,8 +41,8 @@
using testing::UnorderedElementsAreArray;
class IgnoreDiagnostics : public DiagnosticsConsumer {
- void onDiagnosticsReady(
- PathRef File, Tagged> Diagnostics) override {}
+ void onDiagnosticsReady(PathRef File, Tagged Diagnostics) override {
+ }
};
// FIXME: this is duplicated with FileIndexTests. Share it.
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -21,7 +21,7 @@
using ::testing::Pair;
using ::testing::Pointee;
-void ignoreUpdate(llvm::Optional>) {}
+void ignoreUpdate(llvm::Optional) {}
void ignoreError(llvm::Error Err) {
handleAllErrors(std::move(Err), [](const llvm::ErrorInfoBase &) {});
}
@@ -102,20 +102,20 @@
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero());
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
- [&](std::vector) { Ready.wait(); });
+ [&](DiagList) { Ready.wait(); });
S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](DiagList Diags) { ++CallbackCount; });
S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
+ [&](DiagList Diags) {
ADD_FAILURE() << "auto should have been cancelled by auto";
});
S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
- [&](std::vector Diags) {
+ [&](DiagList Diags) {
ADD_FAILURE() << "no diags should not be called back";
});
S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](DiagList Diags) { ++CallbackCount; });
Ready.notify();
}
EXPECT_EQ(2, CallbackCount);
@@ -131,15 +131,15 @@
// FIXME: we could probably use timeouts lower than 1 second here.
auto Path = testPath("foo.cpp");
S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
+ [&](DiagList Diags) {
ADD_FAILURE() << "auto should have been debounced and canceled";
});
std::this_thread::sleep_for(std::chrono::milliseconds(200));
S.update(Path, getInputs(Path, "auto (timed out)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](DiagList Diags) { ++CallbackCount; });
std::this_thread::sleep_for(std::chrono::seconds(2));
S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+ [&](DiagList Diags) { ++CallbackCount; });
}
EXPECT_EQ(2, CallbackCount);
}
@@ -189,15 +189,14 @@
{
WithContextValue WithNonce(NonceKey, ++Nonce);
- S.update(File, Inputs, WantDiagnostics::Auto,
- [Nonce, &Mut, &TotalUpdates](
- llvm::Optional> Diags) {
- EXPECT_THAT(Context::current().get(NonceKey),
- Pointee(Nonce));
-
- std::lock_guard Lock(Mut);
- ++TotalUpdates;
- });
+ S.update(
+ File, Inputs, WantDiagnostics::Auto,
+ [Nonce, &Mut, &TotalUpdates](llvm::Optional Diags) {
+EXPECT_THAT(Context::current().get(NonceKey), Pointee(Nonce));
+
+std::lock_guard Lock(Mut);
+++TotalUpdates;
+ });
}
{
Index: unittests/clangd/Co
