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