[PATCH] D93978: [clangd] DefineOutline doesn't require implementation file being saved

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 5 07:22:29 PST 2021


sammccall added a comment.

Ah, thanks for working on this!

A few thoughts:

- when we're pseudoparsing the file we're going to modify as we do here, using the new content is strictly better, no downside :-)
- the old method here of accessing the content through the FS attached to the AST is a hack
- however I think DraftStore is the wrong abstraction here and VFS is the right one.
  - each tweak shouldn't have to deal with falling back from DraftStore to VFS (and rename also has an implementation of this!)
  - C++ embedders don't have a DraftStore lying around
  - bug: this code won't find a named but unsaved implementation file (since getCorrespondingHeaderOrSource uses VFS)
- (also Tweak::Selection isn't a pure abstraction, we can jam the FS in there instead of passing it around as a separate param everywhere)

So I think the right way to expose this to tweaks is to add a `vfs::FileSystem *` member to `Tweak::Selection`, that allows accessing the current FS (as opposed to the one used for building).
The simplest implementation uses OverlayVFS + InMemoryFS + TUScheduler::getAllFileContents(). This isn't as expensive as it sounds as we need only set the FS pointer for `apply()`.
(We can also implement a FS that copies more lazily, though I suspect it's not worth it)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93978/new/

https://reviews.llvm.org/D93978



More information about the cfe-commits mailing list