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

[Raoul Wols via Phabricator via cfe-commits]

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