clang-rename: check that the source location we find actually has the old name

[vmiklos]

Summary

This more general check could have prevented the specific problem
"getSourceOrder() == -1" guards.

[klimek]

clang-rename/USRLocFinder.cpp

37 ↗

(On Diff #57077)

For constructors, we nowadays usually want to take strings by value and then std::move them into the fields.

75 ↗

(On Diff #57077)

Why startswith as opposed to ==?

clang-rename/USRLocFinder.h

30–33 ↗

(On Diff #57077)

Now that I see this - is there a reason not to use const std::string& for the usr? Also, should we instead use StringRef?

[vmiklos]

clang-rename/USRLocFinder.cpp

75 ↗

(On Diff #57077)

The source range is "foo(0)", the old name is "foo". Or is there a way to get the range of the name part of the initializer, without the "(0)" part?

I'll fix the rest, those are easy. :-)

[klimek]

clang-rename/USRLocFinder.cpp

75 ↗

(On Diff #57077)

Yep, if you want to get an identifier, you just get the code for the range getLocation(), getLocation() (note that ranges are inclusive token-ranges by default)

[vmiklos]

Got rid of startswith() and now using StringRef everywhere instead of a mix of std::string, const std::string and const std::string&.

[klimek]

lg

clang-rename/USRLocFinder.cpp

126–128 ↗

(On Diff #57140)

Usually storing StringRefs in the class is dangerous, but as this is defined in the .cpp file and only used as part of the function, that's probably fine.
(I personally would still make them std::string members, and copy either in the constructor or when passing into the function).

clang-tools-extra/trunk/clang-rename/RenamingAction.cpp

public:
   }
 
   void HandleTranslationUnit(ASTContext &Context) override {
     const auto &SourceMgr = Context.getSourceManager();
     std::vector<SourceLocation> RenamingCandidates;
     std::vector<SourceLocation> NewCandidates;
 
     for (const auto &USR : USRs) {
-      NewCandidates = getLocationsOfUSR(USR, Context.getTranslationUnitDecl());
+      NewCandidates = getLocationsOfUSR(USR, PrevName, Context.getTranslationUnitDecl());
       RenamingCandidates.insert(RenamingCandidates.end(), NewCandidates.begin(),
                                 NewCandidates.end());
       NewCandidates.clear();
     }
 
     auto PrevNameLen = PrevName.length();
     if (PrintLocations)
       for (const auto &Loc : RenamingCandidates) {

clang-tools-extra/trunk/clang-rename/USRLocFinder.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_RENAME_USR_LOC_FINDER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_RENAME_USR_LOC_FINDER_H
 
 #include <string>
 #include <vector>
 
+#include "llvm/ADT/StringRef.h"
+
 namespace clang {
 
 class Decl;
 class SourceLocation;
 
 namespace rename {
 
 // FIXME: make this an AST matcher. Wouldn't that be awesome??? I agree!
-std::vector<SourceLocation> getLocationsOfUSR(const std::string usr,
+std::vector<SourceLocation> getLocationsOfUSR(llvm::StringRef usr,
+                                              llvm::StringRef PrevName,
                                               Decl *decl);
 }
 }
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_RENAME_USR_LOC_FINDER_H

clang-tools-extra/trunk/clang-rename/USRLocFinder.cpp

 //===----------------------------------------------------------------------===//
 
 #include "USRLocFinder.h"
 #include "USRFinder.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Index/USRGeneration.h"
+#include "clang/Lex/Lexer.h"
 #include "llvm/ADT/SmallVector.h"
 
 using namespace llvm;
 
 namespace clang {
 namespace rename {
 
 namespace {
 // \brief This visitor recursively searches for all instances of a USR in a
 // translation unit and stores them for later usage.
 class USRLocFindingASTVisitor
     : public clang::RecursiveASTVisitor<USRLocFindingASTVisitor> {
 public:
-  explicit USRLocFindingASTVisitor(const std::string USR) : USR(USR) {
+  explicit USRLocFindingASTVisitor(StringRef USR, StringRef PrevName) : USR(USR), PrevName(PrevName) {
   }
 
   // Declaration visitors:
 
   bool VisitNamedDecl(const NamedDecl *Decl) {
     if (getUSRForDecl(Decl) == USR) {
       LocationsFound.push_back(Decl->getLocation());
     }
     return true;
   }
 
   bool VisitVarDecl(clang::VarDecl *Decl) {
     clang::QualType Type = Decl->getType();
     const clang::RecordDecl *RecordDecl = Type->getPointeeCXXRecordDecl();
     if (RecordDecl) {
       if (getUSRForDecl(RecordDecl) == USR) {
         // The declaration refers to a type that is to be renamed.
         LocationsFound.push_back(Decl->getTypeSpecStartLoc());
       }
     }
     return true;
   }
 
   bool VisitCXXConstructorDecl(clang::CXXConstructorDecl *ConstructorDecl) {
+    const ASTContext &Context = ConstructorDecl->getASTContext();
     for (clang::CXXConstructorDecl::init_const_iterator it = ConstructorDecl->init_begin(); it != ConstructorDecl->init_end(); ++it) {
       const clang::CXXCtorInitializer* Initializer = *it;
       if (Initializer->getSourceOrder() == -1) {
         // Ignore implicit initializers.
         continue;
       }
 
       if (const clang::FieldDecl *FieldDecl = Initializer->getAnyMember()) {
         if (getUSRForDecl(FieldDecl) == USR) {
           // The initializer refers to a field that is to be renamed.
+          SourceLocation Location = Initializer->getSourceLocation();
+          StringRef TokenName = Lexer::getSourceText(CharSourceRange::getTokenRange(Location), Context.getSourceManager(), Context.getLangOpts());
+          if (TokenName == PrevName) {
+            // The token of the source location we find actually has the old name.
             LocationsFound.push_back(Initializer->getSourceLocation());
           }
         }
       }
+    }
     return true;
   }
 
   // Expression visitors:
 
   bool VisitDeclRefExpr(const DeclRefExpr *Expr) {
     const auto *Decl = Expr->getFoundDecl();
 
while (NameLoc) {
       const auto *Decl = NameLoc.getNestedNameSpecifier()->getAsNamespace();
       if (Decl && getUSRForDecl(Decl) == USR)
         LocationsFound.push_back(NameLoc.getLocalBeginLoc());
       NameLoc = NameLoc.getPrefix();
     }
   }
 
   // All the locations of the USR were found.
-  const std::string USR;
+  StringRef USR;
+  // Old name that is renamed.
+  StringRef PrevName;
   std::vector<clang::SourceLocation> LocationsFound;
 };
 } // namespace
 
-std::vector<SourceLocation> getLocationsOfUSR(const std::string USR,
+std::vector<SourceLocation> getLocationsOfUSR(StringRef USR,
+                                              StringRef PrevName,
                                               Decl *Decl) {
-  USRLocFindingASTVisitor visitor(USR);
+  USRLocFindingASTVisitor visitor(USR, PrevName);
 
   visitor.TraverseDecl(Decl);
   return visitor.getLocationsFound();
 }
 
 } // namespace rename
 } // namespace clang