[PATCH] D44462: [clangd] Don't use DraftMgr in implementation of forceReparse.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/ClangdServer.cpp:143
+  tooling::CompileCommand NewCommand = CompileArgs.getCompileCommand(File);
+  DocVersion Version = DraftMgr.getVersion(File);
+  Path FileStr = File.str();
----------------
simark wrote:
> 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.
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)


================
Comment at: clangd/TUScheduler.h:69
+  /// FIXME: remove the callback from this function
+  void updateCompileCommand(PathRef File, tooling::CompileCommand NewCommand,
+                            IntrusiveRefCntPtr<vfs::FileSystem> FS,
----------------
simark wrote:
> sammccall wrote:
> > (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?)
> 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.
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44462