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