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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clangd/ClangdServer.cpp:338
 
+std::vector<tooling::Replacement>
+ClangdServer::Rename(PathRef File, Position Pos, llvm::StringRef NewName) {
----------------
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.


================
Comment at: clangd/ClangdServer.cpp:347
+
+    void handle(tooling::SymbolOccurrences) override {}
+
----------------
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.


================
Comment at: clangd/ClangdUnit.cpp:1399
+
+SourceLocation getBeginningOfIdentifier(ParsedAST &Unit, const Position &Pos,
+                                        const FileEntry *FE) {
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > nit: the rest of this file defines functions outside their namespace.
> > 
> > TBH I prefer the style you're using here, but we should be consistent within a file.
> +1. Let's document and enforce the preferred style in new commits.
> But better done in a separate commit for all of clangd and let's stick to the original style in this file for now.
Reserved to the original style. Yeah, +1 on change the style for this huge file. Currently, it is not clear for read.


https://reviews.llvm.org/D39676