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