Page MenuHomePhabricator

[clangd] Emit source to Diagnostic.
ClosedPublic

Authored by hokein on Feb 25 2019, 2:40 AM.

Details

Summary

Set "clang", "clang-tidy" as the source to the diagnostics, so that
client can distinguish diagnostics from clang-tidy.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein created this revision.Feb 25 2019, 2:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2019, 2:40 AM
jkorous added inline comments.Feb 26 2019, 3:47 PM
clangd/Diagnostics.cpp
381 ↗(On Diff #188112)

Nit - is this really intended to be part of this patch?

clangd/Diagnostics.h
72 ↗(On Diff #188112)

Nit - is this really intended to be part of this patch?

ilya-biryukov added inline comments.Feb 27 2019, 2:24 AM
clangd/ClangdUnit.cpp
380 ↗(On Diff #188112)

Preamble diagnostics seem to be missing the source.
Could we add a test they also have the "source" set? "unresolved include" should be the easiest to get.

clangd/Diagnostics.h
72 ↗(On Diff #188112)

+1, why do we need ID?

74 ↗(On Diff #188112)

Let's introduce an enum with two values here and convert to string at the LSP boundaries.

hokein updated this revision to Diff 189285.Mar 5 2019, 1:33 AM
hokein marked 6 inline comments as done.

Address comments.

hokein edited reviewers, added: kadircet; removed: ilya-biryukov.Mar 5 2019, 1:35 AM
hokein added a comment.Mar 5 2019, 1:36 AM

add @kadircet as a reviewer, since @ilya-biryukov is OOO.

clangd/ClangdUnit.cpp
380 ↗(On Diff #188112)

good catch, I missed this.

clangd/Diagnostics.cpp
381 ↗(On Diff #188112)

See my comment above.

clangd/Diagnostics.h
72 ↗(On Diff #188112)

Yes, we do need this ID to determine whether a diagnostic is from clang-tidy, CangTidyContext::getCheckName.

kadircet accepted this revision.Mar 5 2019, 2:33 AM

LGTM

This revision is now accepted and ready to land.Mar 5 2019, 2:33 AM
hokein updated this revision to Diff 189455.Mar 6 2019, 1:42 AM

Don't emit the source to LSP level (as this is a UI change as least in vscode)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2019, 2:50 AM