<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Mar 14, 2018, 19:39 Simon Marchi via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">simark added inline comments.<br>
<br>
<br>
================<br>
Comment at: clangd/TUScheduler.h:69<br>
+  /// FIXME: remove the callback from this function<br>
+  void updateCompileCommand(PathRef File, tooling::CompileCommand NewCommand,<br>
+                            IntrusiveRefCntPtr<vfs::FileSystem> FS,<br>
----------------<br>
ilya-biryukov wrote:<br>
> sammccall wrote:<br>
> > simark wrote:<br>
> > > sammccall wrote:<br>
> > > > (summarizing offline discussion)<br>
> > > ><br>
> > > > this is so close to `update`, it'd be nice if we could just call `update` instead.<br>
> > > ><br>
> > > > For that we need the contents, so forceReparse needs contents, so... can forceReparse just be addDocument(skipCache=true) or something?<br>
> > > ><br>
> > > > 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).<br>
> > > ><br>
> > > > reparseOpenFiles needs to move to clangdlspserver, but this seems consistent with the design. (so I think we can drop getTrackedFiles?)<br>
> > > 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.<br>
> > 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.<br>
> > 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.<br>
> Also makes the implementation of `update` a bit simpler, because it doesn't have to care about missing files.<br>
> 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.<br>
> Requiring contents for `forceReparse` seems easier. And when we got automated tracking of changes to compilation database, this will go away<br>
Do you mean `forceReparse` will go away?  Won't it still be required for when the user manually changes the compilation database path?<br></blockquote></div></div><div dir="auto">Tracking changes to files and compile commands means revisiting the abstractions around build systems and file systems. We'll need some way to deal with that case, but it probably shouldn't require lspserver to explicitly tell clangdserver to rebuild.</div><div dir="auto"><br></div><div dir="auto">While on this subject though, I never understood why the "move compilation database" extension was added instead of just restarting clangd - what's usefully preserved?</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D44462" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D44462</a><br>
<br>
<br>
<br>
</blockquote></div></div></div>