Page MenuHomePhabricator

[clangd] Support relatedInformation in diagnostics.
ClosedPublic

Authored by sammccall on Apr 4 2019, 7:15 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Apr 4 2019, 7:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2019, 7:15 AM
ilya-biryukov added inline comments.Apr 10 2019, 3:20 AM
clangd/Diagnostics.cpp
185 ↗(On Diff #193708)

NIT: the comment looks open to misinterpretation, "no structured link" refers to the fact the clients don't support it, but could be read that we don't know which notes correspond to a main diagnostic.

Maybe rephrase to something link "If the client does not support structured links …"?

280 ↗(On Diff #193708)

NIT: maybe call OutFn and return here to avoid checking for EmitRelatedLocations again?
Would arguably make the code simpler, although would require another call to OutFn(Main) outside the if branch.

clangd/SourceCode.cpp
310 ↗(On Diff #193708)

NIT: seems unrelated. Maybe revert?

unittests/clangd/DiagnosticsTests.cpp
62 ↗(On Diff #193708)

Maybe omit the std::tie()-based comparison altogether?
This would not change the semantics of the matcher, right?

259 ↗(On Diff #193708)

maybe use testPath() here to avoid PP directives?

ilya-biryukov added inline comments.Apr 10 2019, 3:22 AM
clangd/Diagnostics.cpp
271 ↗(On Diff #193708)

Maybe vlog? This is what we use for dropped diagnostics, should probably stick to the same level with dropped notes (even though the dropped notes probably come up less often in practice).

kadircet added inline comments.Apr 10 2019, 3:31 AM
clangd/Protocol.cpp
280 ↗(On Diff #193708)

s/CodeActions/RelatedInfoSupport/

ilya-biryukov added inline comments.Apr 10 2019, 3:33 AM
clangd/Protocol.cpp
280 ↗(On Diff #193708)

copy-paste detected? :-)))

sammccall marked 9 inline comments as done.Apr 18 2019, 2:18 AM
sammccall added inline comments.
clangd/Diagnostics.cpp
280 ↗(On Diff #193708)

Yeah, I don't really see this as an improvement - it reduces the nesting, but makes the relation of conditions to code more confusing, I think.

unittests/clangd/DiagnosticsTests.cpp
259 ↗(On Diff #193708)

Done - this requires changing the actual paths but it doesn't seem to matter.

sammccall updated this revision to Diff 195694.Apr 18 2019, 2:18 AM
sammccall marked an inline comment as done.

Rebase and address comments.

sammccall updated this revision to Diff 195706.Apr 18 2019, 3:26 AM

Propagate the capability to Diagnostics, add lit test.

sammccall updated this revision to Diff 195707.Apr 18 2019, 3:27 AM

Remove accidental copy/paste in lit test.

Harbormaster completed remote builds in B30724: Diff 195707.
kadircet added inline comments.Apr 18 2019, 4:19 AM
clangd/Diagnostics.cpp
417 ↗(On Diff #195707)

Do we still need the File field as well?

271 ↗(On Diff #193708)

We seem to be dropping these only at related information case, what about flattening?
Maybe we should get rid of them at that stage as well.

sammccall marked 2 inline comments as done.Apr 18 2019, 4:25 AM
sammccall added inline comments.
clangd/Diagnostics.cpp
417 ↗(On Diff #195707)

It's more readable than full path - e.g. if the main TU is foo.cc and includes foo.h, this is "foo.h".

It's true that if we want to flatten as another pass, we're not going to make use of this information. I'd rather change that in the flatten-as-another-pass patch, so we can see whether the damage to output is worth the refactoring.
(And whether we want to explicitly compute a nice path somehow, etc)

271 ↗(On Diff #193708)

As with the other comment - you're right, and we'll drop these if we refactor - I don't think there's any need to drop them now, though.

Discussed further offline - it's not clear that expressing the flattening as LSP diagnostic -> LSP diagnostic is better than the current Diag -> LSP diagnostic.

So that followup probably won't happen, and there isn't that much to be gained from "unifying" the behavior in !AbsFile or File!=AbsFile cases.

kadircet accepted this revision.Apr 18 2019, 8:02 AM
This revision is now accepted and ready to land.Apr 18 2019, 8:02 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2019, 8:15 AM