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