[PATCH] D37291: [refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.

LGTM.

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



================
Comment at: include/clang/Tooling/Refactoring/RefactoringResultConsumer.h:30
+  /// the source selection has no overlap at all with any relevant AST nodes.
+  virtual void handleInitiationFailure() = 0;
+
----------------
Just wondering if it is possible to provide more information about the initiation failure to the handler?


================
Comment at: unittests/Tooling/RefactoringActionRulesTest.cpp:39
+  class Consumer final : public RefactoringResultConsumer {
+    void handleInitiationFailure() {
+      Result = Expected<Optional<AtomicChanges>>(None);
----------------
arphaman wrote:
> ioeric wrote:
> > 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.
> 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.
Ok. Makes sense.


https://reviews.llvm.org/D37291