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

[Sam McCall via cfe-commits]

On Wed, Mar 14, 2018, 19:39 Simon Marchi via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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?
>
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.

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?

>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D44462
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180314/ed489408/attachment-0001.html>