[PATCH] D39430: [clangd] formatting: don't ignore style

[Raoul Wols via Phabricator via cfe-commits]

rwols marked 4 inline comments as done.
rwols added a comment.

Is this ready to merge? I'd like to implement tests in another differential, I'm having trouble referencing a temp dir from `lit` in a JSON request to clangd.



================
Comment at: clangd/ClangdServer.cpp:429
+  auto TaggedFS = FSProvider.getTaggedFileSystem(File);
+  auto StyleOrError =
+      format::getStyle("file", File, "LLVM", Code, TaggedFS.Value.get());
----------------
sammccall wrote:
> getStyle is going to stat several files, in the usual case.
> This seems less than ideal, particularly when we're doing format-on-type. Filesystems are not always fast/cheap.
> 
> We should really cache this, and I think "when the file is opened" is a reasonable tradeoff between simplicity and producing sensible user-facing behavior.
> 
> Unfortunately I'm not sure there's a sensible place to do things at present: addDocument() is fast and doing blocking IO there might be bad. Doing it asynchronously also seems bad (what happens if style's not ready?)
> 
> Next best thing would be "first format request when the file is opened". If you feel like doing that here (only computing the style if it's not already known for this file), then feel free, but a **FIXME** is fine too.
> 
> (FWIW, Google has a slow FS that's important to us, but we disable .clang-format scanning on that FS in other ways because it only holds code in one style. Other users might care about this, though)
> but a FIXME is fine too.

I've added a FIXME note. I think implementing a caching strategy is worthy of its own differential revision.


https://reviews.llvm.org/D39430