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

[Manuel Klimek via Phabricator via cfe-commits]

klimek 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; }
+
----------------
arphaman wrote:
> 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.
Cool, looking forward to it :)


Repository:
  rL LLVM

https://reviews.llvm.org/D36075