arphaman added inline comments.
================
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:101
+ std::string NewQualifiedName) {
+ return QualifiedRenameRule(std::move(OldQualifiedName),
+ std::move(NewQualifiedName));
----------------
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.
================
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:104
+ if (!ND)
+ return llvm::make_error<llvm::StringError>("Could not find symbol " +
+ OldQualifiedName,
----------------
You can use a refactoring diagnostic here (see `RenameOccurrences::initiate`).
https://reviews.llvm.org/D39332
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
