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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Impl LG apart from error handling :-)



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


================
Comment at: clangd/ClangdServer.cpp:345
+  public:
+    void handleError(llvm::Error Err) override {}
+
----------------
don't you want to do something here?


================
Comment at: clangd/ClangdServer.cpp:347
+
+    void handle(tooling::SymbolOccurrences) override {}
+
----------------
I don't think you need to override this, assuming you don't expect any of these


================
Comment at: clangd/ClangdServer.cpp:369
+        Context, SourceRange(SourceLocationBeg), NewName.str());
+    if (rename)
+       rename->invoke(ResultCollector, Context);
----------------
else?


================
Comment at: clangd/ClangdServer.cpp:377
+      // FIXME: Right now we only support renaming the main file, so we drop
+      // replacements not for the main file.
+      if (Rep.getFilePath() == File)
----------------
Add to the comment what you actually want to fix :-)
Things we may want:
 - rename in any included header
 - rename only in the "main" header
 - provide an error if there are decls we won't rename (e.g. if you rename std::vector)
 - rename globally in project (this may be slow or surprising, so I'd make that a more explicit action)
 - rename in open files (though I'm leery about side-effects of open files)


================
Comment at: clangd/ClangdUnit.cpp:1399
+
+SourceLocation getBeginningOfIdentifier(ParsedAST &Unit, const Position &Pos,
+                                        const FileEntry *FE) {
----------------
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.


================
Comment at: test/clangd/rename.test:1
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
----------------
For new tests after rL317486, please add -pretty and FileCheck -strict-whitespace

(See that patch for how the CHECKs should look)


================
Comment at: test/clangd/rename.test:14
+#
+
+Content-Length: 159
----------------
uber-nit: the # line previous to this one is for spacing, no need for another blank line (or any of them, as far as I'm concerned)


================
Comment at: test/clangd/rename.test:25
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+# CHECK: {"jsonrpc":"2.0","id":3,"result":null}
+Content-Length: 33
----------------
No need to assert the response from shutdown (I removed most of these recently)


https://reviews.llvm.org/D39676