[PATCH] D54077: [clangd] Implemented DraftFileSystem

[Sam McCall via cfe-commits]

Disclaimer: I'm on a train with family today, and haven't actually read the
patch...

So I have concerns :-)

1. There's the usual concern that the current behavior is reasonable and
people like it, so adding a second reasonable behavior provides a small
amount of value to the userbase as a whole (because 99%+ will use the
default). If the benefit is small, the comparison to support cost may be
unfavorable.

2. This needs to invalidate preambles more often, which brings both
performance and complexity questions. E.g. we will at some point invalidate
preambles and regenerate diagnostics based on file writes. After this
change, we'll be obligated to do so for edits too. (Remember, LSP has no
concept of "the foreground file"). Invalidating a preamble is expensive,
and edits come rapidly and may invalidate multiple TUs. The current
behavior seems more compatible with being both simple and fast.

So mostly I'd like to be convinced that this is important (or that it's
simple, but that seems unlikely at first glance).

Cheers, Sam

On Mon, Nov 5, 2018, 11:32 Ilya Biryukov via Phabricator <
reviews at reviews.llvm.org wrote:

> ilya-biryukov added subscribers: simark, klimek.
> ilya-biryukov added a comment.
>
> Thanks for the patch! I believe many people I talked to want this behavior
> (myself included).
> Some people like what we do now more. It feels like it depends on the
> workflow: for people who auto-save *all* files  before build (some editors
> do it automatically?) the new behavior is the right one, for people who
> only save current file and rerun build on the console the old behavior is
> the correct one.
> It all boils down to the argument "I want to see same errors that running
> the compiler would produce".
>
> @klimek, @arphaman, @simark, WDYT?
>
>
>
> ================
> Comment at: clangd/FS.h:82
> +
> +    if (auto D = DS.getDraft(Path.str())) {
> +      return std::unique_ptr<llvm::vfs::File>(
> ----------------
> This assumes the `Path` is absolute and `vfs::FileSystem` can be called
> with non-absolute paths too.
> One way to make it work with relative paths is to create an
> `InMemoryFileSystem` with the headers (it handles the absolute paths
> conversions) and create an `OverlayFileSystem` on top of it and the
> `RealFileSystem`.
>
> This would also make more complicated things work, e.g. getting the same
> files when traversing parent directories, etc.
>
> Could we try this approach? WDYT?
>
>
>
> Repository:
>   rCTE Clang Tools Extra
>
> https://reviews.llvm.org/D54077
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181105/ecdb761c/attachment.html>