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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
