[PATCH] D44142: [clangd] Revamp handling of diagnostics.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/Diagnostics.h:35
+  DiagnosticsEngine::Level Severity;
+  llvm::SmallVector<TextEdit, 1> 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<const Diag &> {
+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<const clangd::Diag &>
----------------
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