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

[Simon Marchi via Phabricator via llvm-commits]

simark added inline comments.


================
Comment at: clangd/TUScheduler.h:69
+  /// FIXME: remove the callback from this function
+  void updateCompileCommand(PathRef File, tooling::CompileCommand NewCommand,
+                            IntrusiveRefCntPtr<vfs::FileSystem> FS,
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > 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.
> > 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
Do you mean `forceReparse` will go away?  Won't it still be required for when the user manually changes the compilation database path?


Repository:
  rL LLVM

https://reviews.llvm.org/D44462