arphaman marked 5 inline comments as done.
arphaman added inline comments.
================
Comment at: include/clang/Tooling/Refactoring/Extract/ExtractFunction.h:21
+/// then called from the place where the original code was.
+class ExtractFunction final : public SourceChangeRefactoringRule {
+public:
----------------
sammccall wrote:
> The interface given here looks great, much simpler to use directly!
>
> The inheritance gives me some pause. A couple of related things are going on
> here:
> - we're inheriting the `RefactoringActionRuleBase` interface, which allows
> this refactoring to be used generically
> - we're inheriting `SourceChangeRefactoringRule`, which adapts the generic
> interface to the one we actually implement
>
> As consequences:
> - we're bound to use a fairly generic name as our entry point here (but I
> actually think it's a good name, so no worries)
> - we advertise *two* public entry point, that do the same thing (but it's
> not obvious from the signature)
>
> The thing is, I don't think people who want the "generic" interface will be
> using this class directly - they'll be going through the Engine, right?
> So `ExtractFunction` -> `RefactoringActionRuleBase` adapter doesn't need to
> be here, I think it can be private in the Engine.
> That seems like a clearer separation of concerns: `ExtractFunction` only
> cares about getting its work done, and `RefactoringEngine` owns exposing a
> generic interface to all the rules.
You're right about the dual adaption, but this kind of class **should not** be
invoked directly for the following reason:
While it's easy to call `createSourceReplacements` or `invoke` for local
refactorings, the cross-TU actions will need a different invocation harness
around them. Clangd should avoid using this kind of class directly and use
another harness that can run refactorings across multiple-TUs if needed.
I think it's best if it stays right now, as it will help to drive cross-TU
refactorings. We can reconsider this in the future though.
I made the `createSourceReplacements` here private though.
================
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:79
+ /// Returns the descriptor for this refactoring rule.
+ virtual const RefactoringDescriptorInterface &getDescriptor() = 0;
};
----------------
sammccall wrote:
> Did you intend to put this in RefactoringActionRuleBase? It'd be nice to have
> it for all types of rules.
>
> Also, it's an instance method, but don't you want to be able to get at the
> information even if you can't apply the refactoring in the given context (so
> initiate fails).
>
> Maybe this should be static (and `RefactoringEngine` should expose it as part
> of its generic interface)
Classes like ExtractFunction could be reused by multiple rules, so I put it
there before to keep the "is this refactoring available in editor" associated
with a rule. But now with a more generic descriptor that doesn't really
associate title with "is this refactoring available in an editor" I can put the
descriptor into the ExtractFunction class itself.
Repository:
rL LLVM
https://reviews.llvm.org/D38985
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
