Page MenuHomePhabricator

[clangd] Don't use DraftMgr in implementation of forceReparse.
ClosedPublic

Authored by ilya-biryukov on Mar 14 2018, 4:52 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Mar 14 2018, 4:52 AM
simark accepted this revision.Mar 14 2018, 6:57 AM
simark added a subscriber: simark.

I rebased my patch (D44408) on top of this one, it looks good.

clangd/ClangdServer.cpp
143 ↗(On Diff #138329)

I was wondering if we should increment the version here. From what I understand, it is used to identify each version of the parse, so that when diagnostics are ready, we know if they are for the latest version (and should be sent to the user) or if they are stale and should be ignored. If we don't change the document content but change the compile commands, I would consider it as a new version, since the diagnostics coming from the same document but old compile commands should be considered stale.

clangd/TUScheduler.h
62 ↗(On Diff #138329)

"trakced"

This revision is now accepted and ready to land.Mar 14 2018, 6:57 AM

This patch is the right direction, but if you're up for it we can simplify interfaces further I think.

clangd/TUScheduler.h
62 ↗(On Diff #138329)

tracked

69 ↗(On Diff #138329)

(summarizing offline discussion)

this is so close to update, it'd be nice if we could just call update instead.

For that we need the contents, so forceReparse needs contents, so... can forceReparse just be addDocument(skipCache=true) or something?

Requiring content to be passed doesn't seem like a big burden in practice, and makes it clear that clangdserver is never responsible for maintaining copies of the content on the callers behalf (and clangdlspserver is).

reparseOpenFiles needs to move to clangdlspserver, but this seems consistent with the design. (so I think we can drop getTrackedFiles?)

simark added inline comments.Mar 14 2018, 7:33 AM
clangd/TUScheduler.h
69 ↗(On Diff #138329)

I also thought it would be nice to have only one method update. What about if the Contents member of ParseInputs is optional? When it is not instantiated (such as when calling forceReparse), it would mean to re-use the previously sent source.

sammccall added inline comments.Mar 14 2018, 7:45 AM
clangd/ClangdServer.cpp
143 ↗(On Diff #138329)

Diagnostics happen on the ASTWorker thread, of which there's only one per TU, so they're actually already serialized along with the decision of whether to publish.

The only "gap" is that if we add/remove/add the same document, we can get two concurrent astworkers that might deliver diagnostics out-of-order. That's on me to fix, at which point we can remove version comparisons from clangdserver entirely.

(We may still want a version concept - LSP has them, and it's a useful building block. One option is to use them only in ClangdLSPServer and propagate using context. This fails if ClangdServer starts watching disk for file changes, so maybe we'll want to explicitly pass opaque version tokens around at some point. But it's unrelated to the way versions are currently used)

clangd/TUScheduler.h
69 ↗(On Diff #138329)

That would also work. If we can get away with just requiring the contents to always be passed in, I think it's simpler to understand.

ilya-biryukov marked 2 inline comments as done.
  • Remove TUScheduler::updateCompileCommands, add a new optional parameter to clear the cache of compile commands instead
  • Address review comments
ilya-biryukov added inline comments.Mar 14 2018, 9:42 AM
clangd/TUScheduler.h
69 ↗(On Diff #138329)

That would also work. If we can get away with just requiring the contents to always be passed in, I think it's simpler to understand.

Also makes the implementation of update a bit simpler, because it doesn't have to care about missing files.
And ParseInputs is used in other places too, all of them need to be updated or we need two versions of ParseInputs: with optional and required contents.
Requiring contents for forceReparse seems easier. And when we got automated tracking of changes to compilation database, this will go away

sammccall accepted this revision.Mar 14 2018, 9:59 AM
sammccall added inline comments.
clangd/TUScheduler.cpp
220 ↗(On Diff #138394)

you might want to inline this function here again - it's not overly long, and we're now back to one callsite

unittests/clangd/SyncAPI.h
23 ↗(On Diff #138394)

it's slightly odd that wantdiagnostics is missing here. Previously this was "forwards compatible" and could easily be added later. But now we'll have callsites that pass different arguments than addDocument takes.

Maybe just add the defaulted param to the signature?

ilya-biryukov marked 2 inline comments as done.
  • Add WantDiagnostics param to runAddDocument
  • Revert changes in TUScheduler
unittests/clangd/SyncAPI.h
23 ↗(On Diff #138394)

Done. I thought it's slightly confusing to have both Auto and Yes for synchronous case, but being consistent with addDocument is a good cause to do this.

This revision was automatically updated to reflect the committed changes.
simark added inline comments.Mar 14 2018, 11:38 AM
clangd/TUScheduler.h
69 ↗(On Diff #138329)

Do you mean forceReparse will go away? Won't it still be required for when the user manually changes the compilation database path?