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