[PATCH] D36075: [refactor] Initial support for refactoring action rules

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h:23-37
+/// A common refactoring action rule interface.
+class RefactoringActionRule {
+public:
+  enum RuleKind { SourceChangeRefactoringRuleKind };
+
+  RuleKind getRuleKind() const { return Kind; }
+
----------------
klimek wrote:
> Sorry for being late, was out on vacation.
> Generally, why do we need this tag-based abstraction here instead of using the more typical OO double-dispatch where necessary?
> (We do this in the AST a lot, but the AST is special, because there we want to implement a lot of different algorithms that rely on the node type, while I don't see how that applies here)
Generally the clients will have to somehow distinguish between the types of results that are produced by rules to figure out what to do (e.g. AtomicChanges -> apply, SymbolOccurrences -> ask user, Continuation -> look for more ASTs). So far I've thought that the LLVM based dynamic casts will work well for this, e.g. 

```
if (auto *Action = dyn_cast<SourceChangeRefactoringRule>()) {
  Expected<Optional<AtomicChanges>> Changes = Action->createSourceReplacements();
  applyChanges(Changes);
} else if (...) {
  ...
} else (...) {
   ...
}
```

But you're probably right, there might be a better way to do this rather than the tag based approach. Something like a consumer class that clients can implement that provides consumer functions that take in the specific results. I reckon a single consumer will actually work better in the long-run when we might Continuations that both return changes in the first TU and information for searches in other TUs. I'll see if I can get a patch out that removes this tag and uses the consumer approach.


Repository:
  rL LLVM

https://reviews.llvm.org/D36075