[PATCH] D39676: [clangd] Add rename support.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/ClangdServer.cpp:338
 
+std::vector<tooling::Replacement>
+ClangdServer::Rename(PathRef File, Position Pos, llvm::StringRef NewName) {
----------------
hokein wrote:
> sammccall wrote:
> > I think you can split out a private method:
> > 
> >     Expected<vector<Replacement>> ClangdServer::applyRefactoring(RefactoringActionRuleBase&);
> > 
> > This would make this function easier to read by separating out the rename-specific stuff from (some of) the boilerplate.
> > (It also seems likely to be reusable if we add direct support for other refactorings, but that's not the main reason I think)
> I'd keep the code in `Rename` currently, as most of the code is rename-specific, we can refactor it out when the need arises.
OK, then please find some other way to simplify/break up this method - it's too long and complicated.


================
Comment at: clangd/ClangdServer.cpp:347
+
+    void handle(tooling::SymbolOccurrences) override {}
+
----------------
hokein wrote:
> sammccall wrote:
> > I don't think you need to override this, assuming you don't expect any of these
> We have to override this method, the default implementation (generating an "unsupported result" error) is not sensible for clangd.
> Added a comment for it.
I don't understand.

My reading was that we expect handle(SymbolOccurrences) to never be called. If it was called, this is an internal problem and reporting an error seems reasonable.

Under what circumstances would it be called *and* the right response would be to ignore it?


================
Comment at: clangd/ClangdServer.cpp:359
+    void handle(tooling::AtomicChanges SourceReplacements) override {
+      Result = std::move(SourceReplacements);
+    }
----------------
the current refactoring engine doesn't call handle() multiple times, nor both handleError() and handle(), but I don't see that guaranteed anywhere.

At least we should assert that we're not overwriting anything.


https://reviews.llvm.org/D39676