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