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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clangd/ClangdServer.cpp:347
+
+    void handle(tooling::SymbolOccurrences) override {}
+
----------------
sammccall wrote:
> 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?
Oops,  you are right. I misread the code of RenameOccurrences: I thought the rename workflow is to find the occurrences,  pass to occurrences to the ResultConsumer, generate the atomic change and pass it to ResultConsumer. But the it turns out it just uses the occurrences to generate atomic change directly.


================
Comment at: clangd/ClangdServer.cpp:359
+    void handle(tooling::AtomicChanges SourceReplacements) override {
+      Result = std::move(SourceReplacements);
+    }
----------------
sammccall wrote:
> 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.
I think the engine should do it (the current refactoring rule implementations imply it). If the refactoring succeeds, call "handle(AtomicChanges)" to pass the result; If any error happened during refactoring, call "handle(Error)".

But adding assert is safer.


https://reviews.llvm.org/D39676