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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.h:289
+  llvm::Expected<std::vector<tooling::Replacement>>
+  formatRange(llvm::StringRef Code, PathRef File, Range Rng);
+
----------------
rwols wrote:
> rwols wrote:
> > ilya-biryukov wrote:
> > > rwols wrote:
> > > > ilya-biryukov wrote:
> > > > > Why do we accept `Code` as a parameter here instead of getting it internally?
> > > > > 
> > > > > Maybe we should consider moving this method out of `ClangdServer`? Its signature looks pretty self-contained now.
> > > > There are a couple intertwined problems:
> > > > 
> > > > 1. replacementsToEdits wants llvm::StringRef Code
> > > > 2. ClangdServer::formatCode wants llvm::StringRef Code
> > > > 3. ClangdServer::getDocument returns an std::string
> > > > 
> > > > So yes, in principle you can call getDocument when you need it for replacementsToEdits, and you can let formatCode itself call getDocument for clang::format::getStyle. But then we create two copies of the document contents for one LSP request.
> > > > 
> > > > If getDocument returned an llvm::StringRef, I'd probably vote for removing the Code argument everywhere and call getDocument as needed.
> > > Oh, I see. Calling `getDocument` twice does not really make sense.  
> > > Maybe we could move a call to `replacementsToEdits` into `formatOnFile` and make it return `vector<TextEdit>`?  Seems to be solving both problems.
> > > 
> > > We could've made `getDocument` return `StringRef`, but we'd have to be more careful to ensure it's actually copied when we're scheduling async operations, worth a separate review.
> > > Maybe we could move a call to replacementsToEdits into formatOnFile and make it return vector<TextEdit>?
> > 
> > I disagree, this will bring LSP-specific protocols into `ClangdServer`. The translation from `tooling::Replacement` to `clangd::TextEdit` should remain in `ClangdLSPServer`.
> I tried this anyway, but `ClangdLSPServer::getFixIts` also uses `replacementsToEdits`. And that maintains a map with `tooling::Replacement`s. So, if we want to move `replacementsToEdits` into `ClangdServer`, we would have to move that map into `ClangdServer` too I think.
> I disagree, this will bring LSP-specific protocols into ClangdServer. The translation from tooling::Replacement to clangd::TextEdit should remain in ClangdLSPServer.
+1 to this, I'd also not make `ClangdServer` LSP-specific. However we already use some LSP structs from `Protocol.h` there to avoid code duplication (i.e., `CompletionItem`, though LSP-specific, suits `ClangdServer` pretty well). In principle, `Protocol.h` should not be a dependency, though, but we try to manage what we take from there instead. `clangd::TextEdit` seems to be in line with other things we take from there now (that is, `Range`s, `Location`s, etc)

> I tried this anyway, but ClangdLSPServer::getFixIts also uses replacementsToEdits. And that maintains a map with tooling::Replacements. So, if we want to move `replacementsToEdits` into `ClangdServer`, we would have to move that map into ClangdServer too I think.
Could we change the `map` to store `clangd::TextEdit` instead? It's a bit unfortunate that we'll be making unnecessary `tooling::Replacement` -> `clangd::TextEdit` conversions and those aren't free. But honestly, I don't think this should bite us performance-wise.
Given that we already use LSP's `Location` and `Range` encoding in other APIs, we'll get a more consistent interface.


https://reviews.llvm.org/D39430