ioeric added a comment.
I think this is a great start!
First round with some nits.
================
Comment at: clang-rename/RenamingAction.cpp:87
+class QualifiedRenamingASTConsumer : public ASTConsumer {
+public:
----------------
Comments.
================
Comment at: clang-rename/RenamingAction.h:45
+class QualifiedRenamingAction {
+public:
----------------
Comments. Same for other classes.
================
Comment at: clang-rename/RenamingAction.h:60
+ const std::vector<std::string> &OldNames;
+ const std::vector<std::vector<std::string>> &USRList;
+ std::map<std::string, tooling::Replacements> &FileToReplaces;
----------------
What is `USRList`?
================
Comment at: clang-rename/USRFinder.cpp:139
+ if (Name != Decl->getQualifiedNameAsString() &&
+ Name != "::" + Decl->getQualifiedNameAsString()) {
return true;
----------------
nit: No braces for one-liners.
================
Comment at: clang-rename/USRFinder.cpp:200
+ // Also find all USRs of nested declarations.
+ NestedNameSpecifierLocFinder Finder(const_cast<ASTContext &>(Context));
----------------
It is unclear to me what `nested declarations` are.
================
Comment at: clang-rename/USRLocFinder.cpp:152
+clang::SourceLocation StartLocationForType(clang::TypeLoc TL) {
+ // For elaborated types (e.g. `struct a::A`) we want the portion after the
----------------
You don't need "clang::" here and elsewhere.
================
Comment at: clang-rename/USRLocFinder.cpp:156
+ if (TL.getTypeLocClass() == clang::TypeLoc::Elaborated) {
+ clang::NestedNameSpecifierLoc nested_name_specifier =
+ TL.castAs<clang::ElaboratedTypeLoc>().getQualifierLoc();
----------------
Variable names should be LLVM style.
================
Comment at: clang-rename/USRLocFinder.cpp:157
+ clang::NestedNameSpecifierLoc nested_name_specifier =
+ TL.castAs<clang::ElaboratedTypeLoc>().getQualifierLoc();
+ if (nested_name_specifier.getNestedNameSpecifier())
----------------
Is this cast always safe?
================
Comment at: clang-rename/USRLocFinder.cpp:169
+ TL.getTypeLocClass() == clang::TypeLoc::Qualified) {
+ TL = TL.getNextTypeLoc();
+ }
----------------
nit: No braces around one-liners. Same below.
================
Comment at: clang-rename/USRLocFinder.cpp:196
+
+class RenameLocFindingASTVisitor
+ : public clang::RecursiveASTVisitor<RenameLocFindingASTVisitor> {
----------------
Comments.
================
Comment at: clang-rename/USRLocFinder.cpp:196
+
+class RenameLocFindingASTVisitor
+ : public clang::RecursiveASTVisitor<RenameLocFindingASTVisitor> {
----------------
ioeric wrote:
> Comments.
I think this visitor deserves a file of its own.
================
Comment at: clang-rename/USRLocFinder.cpp:368
+private:
+ bool IsTypeAliasWhichWillBeRenamedElsewhere(TypeLoc TL) const {
+ while (!TL.isNull()) {
----------------
Note this is not expected behavior for alias types which would always be
skipped in this check.
================
Comment at: clang-rename/USRLocFinder.cpp:466
+
+ for (const auto &Loc : Finder.getRenameInfos()) {
+ std::string ReplacedName = NewName.str();
----------------
`Loc` seems to be a bad name here. It is usually used for TypeLoc.
================
Comment at: clang-rename/USRLocFinder.h:34
+std::vector<NewNameReplacement>
+getQualifiedRenameLocsOfUSRs(llvm::ArrayRef<std::string> USRs,
+ llvm::StringRef OldName, llvm::StringRef NewName,
----------------
Comments.
https://reviews.llvm.org/D31176
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
