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