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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks! Mostly just comments around the IO (which I'm still learning about myself).
Otherwise this LG.

I'm afraid you're going to run into some merge conflicts with https://reviews.llvm.org/rL317486 - I think/hope not too bad, though.



================
Comment at: clangd/ClangdServer.cpp:41
   // Call clang-format.
-  // FIXME: Don't ignore style.
-  format::FormatStyle Style = format::getLLVMStyle();
-  auto Result = format::reformat(Style, Code, Ranges, Filename);
-
+  auto StyleOrError = format::getStyle("file", Filename, "LLVM");
+  if (!StyleOrError)
----------------
rwols wrote:
> sammccall wrote:
> > This needs to use the VFS I think. (I'm not familiar with the details of the VFS usage I'm afraid)
> Haven't tried implementing this yet, but from some simple experiments it doesn't seem necessary.
Thanks for doing this. The reason it's needed is we want to run clangd on systems where the source tree is not a real filesystem. Looking on the real filesystem for .clang-format files would be incorrect.


================
Comment at: clangd/ClangdServer.cpp:297
 
-std::vector<tooling::Replacement> ClangdServer::formatRange(PathRef File,
-                                                            Range Rng) {
-  std::string Code = getDocument(File);
-
+llvm::Expected<Tagged<std::vector<tooling::Replacement>>>
+ClangdServer::formatRange(llvm::StringRef Code, PathRef File, Range Rng) {
----------------
As I understand, we tag the return values to ensure we get consistent reads, and describe the consistency of those reads, on systems that support that (e.g. snapshot filesystems).

I don't think we need to do that here, because:
 - formatting only depends on the formatted file, and .clang-format
 - the formatted file's contents are passed directly
 - we should aggressively cache .clang-format reads, I think (see below)


================
Comment at: clangd/ClangdServer.cpp:429
+  auto TaggedFS = FSProvider.getTaggedFileSystem(File);
+  auto StyleOrError =
+      format::getStyle("file", File, "LLVM", Code, TaggedFS.Value.get());
----------------
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)


================
Comment at: clangd/ClangdServer.h:314
 private:
+  // FIXME: We don't need the Code argument, but that data just so happens to
+  // be present when invoking this method.
----------------
I don't understand this comment - I think we require and use Code.

If you mean we can just read Code from disk, that won't work: we want to format the dirty buffer in memory, not the last saved version.


https://reviews.llvm.org/D39430