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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.


================
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:21
+struct RefactoringResult {
+  enum ResultKind {
+    /// A set of source replacements represented using a vector of
----------------
ioeric wrote:
> arphaman wrote:
> > ioeric wrote:
> > > I'm a bit unsure about the abstraction of the refactoring result. I would expected refactoring results to be source changes always. Do you have any refactoring tool that outputs occurrences in mind?
> > In Xcode we require rename to return symbol occurrences because the IDE is responsible for figuring out: 
> > 1) The new name. Thus we can't produce replacements when renaming because we don't know what the new name is when gathering the occurrences.
> > 2) Whether these occurrences should be actually applied. Thus we can't produce replacements because it's up to the user to decide if they want to rename some occurrence in a comment for example.
> > 
> > In general 2) can be applied to tools like clang-refactor that could allow users to select occurrences that don't guarantee a direct semantic match (comments, etc.) in an interactive manner.
> > 
> > I think clangd has a rather naive rename support, so these points may not apply, but we may want to extend clangd's support for rename in the future as well.
> I feel occurrences are more of an intermediate state of a refactoring action than a result. I'm wondering if it makes sense to introduce a separate class to represent such intermediate states? I am a bit nervous to fuse multiple classes into one; the interfaces can get pretty ugly when more result kinds are added. 
Good point. I agree.

I think it would be better to differentiate between `RefactoringActionRules` then. Ordinary rules return a set of AtomicChanges instead of RefactoringResult. But then we could also have "interactive" rules that return "partial" results like symbol occurrences.

I think I'll try the following approach:

```
class RefactoringActionRule {
  virtual ~RefactoringActionRule() {}
};

class RefactoringActionSourceChangeRule: public RefactoringActionRule {
public:
  virtual Expected<Optional<AtomicChanges>>
  createSourceReplacements(RefactoringOperation &Operation) = 0;
};

class RefactoringActionSymbolOccurrencesRule: public RefactoringActionRule {
public:
  virtual Expected<Optional<SymbolOccurrences>>
  findSymbolOccurrences(RefactoringOperation &Operation) = 0;
};
```


Repository:
  rL LLVM

https://reviews.llvm.org/D36075