This is an archive of the discontinued LLVM Phabricator instance.

[refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes
ClosedPublic

Authored by arphaman on Aug 30 2017, 3:16 AM.

Details

Summary

This patch changes the way that the refactoring results are produced. Instead of using different RefactoringActionRule subclasses for each result type, we now use a single RefactoringResultConsumer. This was suggested by Manuel in https://reviews.llvm.org/D36075.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Aug 30 2017, 3:16 AM
arphaman updated this revision to Diff 113222.Aug 30 2017, 3:26 AM

Fixed a comment

arphaman updated this revision to Diff 113223.Aug 30 2017, 3:34 AM

Use std::move

JonasToth resigned from this revision.Aug 30 2017, 5:13 AM
ioeric added inline comments.Aug 30 2017, 7:51 AM
include/clang/Tooling/Refactoring/RefactoringResultConsumer.h
39 ↗(On Diff #113223)

I think this interface is specific to some refactoring rules and should be pushed down to derived classes.

unittests/Tooling/RefactoringActionRulesTest.cpp
39 ↗(On Diff #113223)

Can we probably have default error handling in the base class so that we don't need to re-implement these for every derived consumer. I would expect the error handling for initiation and invocation to be similar in different consumers.

arphaman added inline comments.Aug 30 2017, 8:20 AM
include/clang/Tooling/Refactoring/RefactoringResultConsumer.h
39 ↗(On Diff #113223)

Are you talking about derived classes of RefactoringResultConsumer? So something like

class RefactoringResultConsumer {
  virtual void handleInvocationError(llvm::Error Err) = 0;
};

class RefactoringResultSourceReplacementConsumer: RefactoringResultConsumer {
 virtual void handle(AtomicChanges SourceReplacements) = 0;
};

If yes, can you please clarify how the rule can call handle if it's in a subclass of RefactoringResultConsumer?

ioeric added inline comments.Aug 31 2017, 5:51 AM
include/clang/Tooling/Refactoring/RefactoringResultConsumer.h
39 ↗(On Diff #113223)

Sorry, I thought the handle interface was dispatched by template.

Maybe we can have default implementation of rule-specific handlers, e.g. generate errors, in the base class? Derived classes can implement handlers that they care about and ignore others.

arphaman added inline comments.Aug 31 2017, 6:18 AM
include/clang/Tooling/Refactoring/RefactoringResultConsumer.h
39 ↗(On Diff #113223)

Sure, I will add default implementations for handle(...).

I'm not sure about handle...Error(Error) though because llvm::Error forces some form of handling, and I don't want to use a generic stub that will silence the llvm::Error as it won't be that useful. The errors will be handled differently by different clients anyway, like clang-refactor might output the error to stderr, and clangd might either try to notify the user or silently swallow the error.

arphaman updated this revision to Diff 113397.Aug 31 2017, 6:43 AM
arphaman marked an inline comment as done.
  • Add default implementation for handle()
  • Merge two error handling functions into one
unittests/Tooling/RefactoringActionRulesTest.cpp
39 ↗(On Diff #113223)

I've merge the two error functions into one, but I'm reluctant to add default implementation for them because of the reasons that I've mentioned in my previous inline comment.

ioeric accepted this revision.Aug 31 2017, 7:15 AM

LGTM.

@klimek Manuel, would you like to take a look? This addresses your comment https://reviews.llvm.org/D36075#854075

include/clang/Tooling/Refactoring/RefactoringResultConsumer.h
30 ↗(On Diff #113397)

Just wondering if it is possible to provide more information about the initiation failure to the handler?

unittests/Tooling/RefactoringActionRulesTest.cpp
39 ↗(On Diff #113223)

Ok. Makes sense.

This revision is now accepted and ready to land.Aug 31 2017, 7:15 AM
klimek edited edge metadata.Aug 31 2017, 7:48 AM

Thanks! I think this makes the code easier to understand. Now my remaining question is why the ResultType is templates vs. also being an interface (sorry for being late, was out on vacation the past 2 weeks :)

include/clang/Tooling/Refactoring/RefactoringResultConsumer.h
30 ↗(On Diff #113397)

Also, why is initiationFailure not simply handled via handleError?

arphaman added inline comments.Aug 31 2017, 9:52 AM
include/clang/Tooling/Refactoring/RefactoringResultConsumer.h
30 ↗(On Diff #113397)

Initiation failure is not really an error as such. It corresponds to a selection in an editor that doesn't overlap with any interesting AST at all. In the editor, the rule will fail to be initiated without a concrete error when we request it, or will be skipped when the editor is querying available actions for some selection. Therefore it doesn't really have any other information.

However, your comments made me reconsider this particular design point. We should emit an error for this kind of initiation failure as well, and clang-refactor will benefit by default as it will consume it and display it. The editor clients will be able to avoid this kind of error if they want by inspecting the selection constraint prior to invocation.

I will remove this method and use just handleError when committing.

Thanks! I think this makes the code easier to understand. Now my remaining question is why the ResultType is templates vs. also being an interface (sorry for being late, was out on vacation the past 2 weeks :)

ResultType is typically a vector of AtomicChange or SymbolOccurrences which we can't subclass from an interface. Or are you wondering if AtomicChange and SymbolOccurrence should subclass from a common interface? It could potentially be useful, but I'm not sure if there could be that many methods that would go into the interface.

This revision was automatically updated to reflect the committed changes.