This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Track document versions, include them with diags, enhance logs
ClosedPublic

Authored by sammccall on Mar 3 2020, 4:31 PM.

Details

Summary

This ties to an LSP feature (diagnostic versioning) but really a lot
of the value is in being able to log what's happening with file versions
and queues more descriptively and clearly.

As such it's fairly invasive, for a logging patch :-\

Key decisions:

  • at the LSP layer, we don't reqire the client to provide versions (LSP makes it mandatory but we never enforced it). If not provided, versions start at 0 and increment. DraftStore handles this.
  • don't propagate magically using contexts, but rather manually: addDocument -> ParseInputs -> (ParsedAST, Preamble, various callbacks) Context-propagation would hide the versions from ClangdServer, which would make producing good log messages hard
  • within ClangdServer, treat versions as opaque and unordered. json::Value is a convenient type for this, and allows richer versions for embedders. They're "mandatory" but nullptr is a sensible default.

Diff Detail

Event Timeline

sammccall created this revision.Mar 3 2020, 4:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2020, 4:31 PM
sammccall updated this revision to Diff 248069.Mar 3 2020, 4:33 PM

Fix accidental default.

sammccall updated this revision to Diff 248070.Mar 3 2020, 4:39 PM

Fix incomplete message.

Harbormaster failed remote builds in B47997: Diff 248066!
Harbormaster failed remote builds in B48001: Diff 248070!
hokein accepted this revision.Mar 4 2020, 1:57 AM

looks good, just a reminder -- the API change of the clangdServer::Callback would break out our internal integration.

This revision is now accepted and ready to land.Mar 4 2020, 1:57 AM
kadircet added inline comments.Mar 4 2020, 2:31 AM
clang-tools-extra/clangd/ClangdServer.h
73

can we rather have Optional<int>s here(both for callbacks and addDocument)?

as clangdserver layer doesn't touch json objects at all currently.

clang-tools-extra/clangd/Compiler.h
49

again this (and other clangdserver structs) can hold a optional<int> right?

sammccall updated this revision to Diff 248351.Mar 4 2020, 4:02 PM

Use string instead of json Value

sammccall marked an inline comment as done.Mar 4 2020, 4:22 PM
sammccall added inline comments.
clang-tools-extra/clangd/ClangdServer.h
73

I really do want to make these opaque at the lower layer.
json is a bit fiddly though, reworked to use strings instead.

This revision was automatically updated to reflect the committed changes.
kadircet added inline comments.Mar 5 2020, 12:27 AM
clang-tools-extra/clangd/ClangdServer.h
73

looks better thanks !