[PATCH] D39332: [clang-refactor] Introduce a new rename rule for qualified symbols

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added inline comments.


================
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:101
+                              std::string NewQualifiedName) {
+  return QualifiedRenameRule(std::move(OldQualifiedName),
+                             std::move(NewQualifiedName));
----------------
arphaman wrote:
> sammccall wrote:
> > hokein wrote:
> > > arphaman wrote:
> > > > ioeric wrote:
> > > > > hokein wrote:
> > > > > > arphaman wrote:
> > > > > > > It might be better to find the declaration (and report error if needed) during in initiation, and then pass the `ND` to the class. Maybe then both `RenameOccurrences` and `QualifiedRenameRule` could subclass from one base class that actually does just this:
> > > > > > > 
> > > > > > > ``` 
> > > > > > > auto USRs = getUSRsForDeclaration(ND, Context.getASTContext());
> > > > > > >   assert(!USRs.empty());
> > > > > > >   return tooling::createRenameAtomicChanges(
> > > > > > >       USRs, NewQualifiedName, Context.getASTContext().getTranslationUnitDecl());
> > > > > > > ```
> > > > > > Done. Introduced a common interface `USRRenameRule`.
> > > > > `USRRenameRule` doesn't seem to be a very useful abstraction. I think what Alex proposed is a base class that implements `createSourceReplacements` which could be shared by both `RenameOccurrences` and `QualifiedRenameRule`. Intuitively, un-qualified rename is a special case of qualified rename (maybe?), where namespace is not changed.
> > > > Yep, I meant that indeed.
> > > Ah, sorry for the misunderstanding. 
> > > 
> > > Unfortunately, `RenameOccurrences` and `QualifiedRenameRule` could not share the same `createSourceReplacements` implementation, 
> > > 
> > > * Their supported use cases are different. `QualifiedRenameRule` only supports renaming class, function, enums, alias and class members, which doesn't cover all the cases of `RenameOccurrences` (like renaming local variable in function body).
> > > 
> > > * we have separated implementations in the code base(`createRenameAtomicChanges` for qualified rename, `createRenameReplacements` for un-qualified rename). The implementation of qualified rename does more things, including calculating the shortest prefix qualifiers, getting correct range of replacement.  
> > > 
> > > 
> > That makes sense (I think), but then we should remove  `USRRenameRule` again if we're not actually reusing anything.
> > 
> > (And ideally we can hide any such future reuse as functions in the cc file, rather than a class hierarchy exposed in the header)
> ok, that's fine for now then.
Done. Reverted it back. 


================
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:104
+  if (!ND)
+    return llvm::make_error<llvm::StringError>("Could not find symbol " +
+                                                   OldQualifiedName,
----------------
arphaman wrote:
> You can use a refactoring diagnostic here (see `RenameOccurrences::initiate`).
I think using StringError is sufficient here -- didn't see much value using diagnostic:

1. we need to add a new diagnostic type `clang_refactor_no_symbol`.
2. we can't include the detailed information (`OldQualifiedName`) in the diagnostic, and we don't have SourceLocation in the code for the diagnostic.


https://reviews.llvm.org/D39332