This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Support relatedInformation in diagnostics.
ClosedPublic

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

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
210–215

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 …"?

316

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
65

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

261

maybe use testPath() here to avoid PP directives?

ilya-biryukov added inline comments.Apr 10 2019, 3:22 AM
clangd/Diagnostics.cpp
307

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

s/CodeActions/RelatedInfoSupport/

ilya-biryukov added inline comments.Apr 10 2019, 3:33 AM
clangd/Protocol.cpp
280

copy-paste detected? :-)))

sammccall marked 9 inline comments as done.Apr 18 2019, 2:18 AM
sammccall added inline comments.
clangd/Diagnostics.cpp
316

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
261

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
307

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.

417–420

Do we still need the File field as well?

sammccall marked 2 inline comments as done.Apr 18 2019, 4:25 AM
sammccall added inline comments.
clangd/Diagnostics.cpp
307

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.

417–420

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)

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