[PATCH] Fix replacements for files with relative paths are not applied.

[Ariel Bernal]

  Added unittest

Hi klimek, tareqsiraj, revane,

http://llvm-reviews.chandlerc.com/D1800

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D1800?vs=4586&id=4641#toc

Files:
  include/clang/Basic/FileManager.h
  lib/Basic/FileManager.cpp
  lib/Tooling/Refactoring.cpp
  unittests/Tooling/RefactoringTest.cpp

Index: include/clang/Basic/FileManager.h
===================================================================
--- include/clang/Basic/FileManager.h
+++ include/clang/Basic/FileManager.h
@@ -273,6 +273,10 @@
   /// required, which is (almost) never.
   StringRef getCanonicalName(const DirectoryEntry *Dir);
 
+
+  /// \brief Check if a FilePath is a virtual file that we have allocated.
+  bool isFileVirtual(StringRef FilePath);
+
   void PrintStats() const;
 };
 
Index: lib/Basic/FileManager.cpp
===================================================================
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -580,6 +580,16 @@
 #endif
 }
 
+bool FileManager::isFileVirtual(StringRef FilePath) {
+  for (SmallVectorImpl<FileEntry *>::const_iterator
+           I = VirtualFileEntries.begin(),
+           E = VirtualFileEntries.end();
+       I != E; ++I)
+    if ((*I)->getName() == FilePath)
+      return true;
+  return false;
+}
+
 void FileManager::PrintStats() const {
   llvm::errs() << "\n*** File Manager Stats:\n";
   llvm::errs() << UniqueRealFiles.size() << " real files found, "
Index: lib/Tooling/Refactoring.cpp
===================================================================
--- lib/Tooling/Refactoring.cpp
+++ lib/Tooling/Refactoring.cpp
@@ -105,15 +105,18 @@
   const std::pair<FileID, unsigned> DecomposedLocation =
       Sources.getDecomposedLoc(Start);
   const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first);
-
-  if (Entry != NULL) {
+  if (Entry != NULL)
     // Make FilePath absolute so replacements can be applied correctly when
-    // relative paths for files are used.
-    llvm::SmallString<256> FilePath(Entry->getName());
-    llvm::error_code EC = llvm::sys::fs::make_absolute(FilePath);
-    // Don't change the FilePath if the file is a virtual file.
-    this->FilePath = EC ? FilePath.c_str() : Entry->getName();
-  } else
+    // relative paths for file are used. But we don't want to change virtual
+    // files.
+    if (Sources.getFileManager().isFileVirtual(Entry->getName())) {
+      this->FilePath = Entry->getName();
+    } else {
+      llvm::SmallString<256> FilePath(Entry->getName());
+      llvm::sys::fs::make_absolute(FilePath);
+      this->FilePath = FilePath.c_str();
+    }
+  else
     this->FilePath = InvalidLocation;
   this->ReplacementRange = Range(DecomposedLocation.second, Length);
   this->ReplacementText = ReplacementText;
Index: unittests/Tooling/RefactoringTest.cpp
===================================================================
--- unittests/Tooling/RefactoringTest.cpp
+++ unittests/Tooling/RefactoringTest.cpp
@@ -244,6 +244,10 @@
     return Context.Sources.createFileID(File, SourceLocation(), SrcMgr::C_User);
   }
 
+  StringRef getFilePath(llvm::StringRef Name) {
+    return TemporaryFiles.lookup(Name);
+  }
+
   std::string getFileContentFromDisk(llvm::StringRef Name) {
     std::string Path = TemporaryFiles.lookup(Name);
     assert(!Path.empty());
@@ -270,6 +274,55 @@
             getFileContentFromDisk("input.cpp"));
 }
 
+TEST_F(FlushRewrittenFilesTest, GetFilePath) {
+  // Create a temporary file.
+  createFile("input.cpp", "line1\nline2\nline3\nline4");
+
+  StringRef FilePath = getFilePath("input.cpp");
+  StringRef TempPath = llvm::sys::path::parent_path(FilePath);
+  StringRef TempFile = llvm::sys::path::filename(FilePath);
+
+  // Save current path.
+  SmallString<512> CurrentPath;
+  llvm::sys::fs::current_path(CurrentPath);
+
+  // Change directory to the temporary directory.
+  EXPECT_FALSE(chdir(TempPath.str().c_str()));
+  llvm::sys::fs::current_path(CurrentPath);
+
+  // Get a FileEntry from the current directory.
+  FileManager Files((FileSystemOptions()));
+  const FileEntry *Entry = Files.getFile(TempFile);
+  ASSERT_TRUE(Entry != NULL);
+
+  // We expect to get just the filename and "." in the FileEntry
+  // since paths are relative to the current directory.
+  EXPECT_EQ(TempFile, Entry->getName());
+  EXPECT_STREQ(".", Entry->getDir()->getName());
+
+  FileID ID = Context.Sources.createFileID(Entry, SourceLocation(),
+                  SrcMgr::C_User);
+
+  Replacement R = Replacement(Context.Sources, Context.getLocation(ID, 2, 1),
+                              5, "replaced");
+
+  // Change back to the original path so we can verify that replacements
+  // are being applied independently of the location.
+  EXPECT_FALSE(chdir(CurrentPath.c_str()));
+
+  // We expect that the file path of the replacement is using the absolute
+  // path.
+  EXPECT_EQ(R.getFilePath(), getFilePath("input.cpp"));
+
+  // Apply replacements.
+  Replacements Replaces;
+  Replaces.insert(R);
+  EXPECT_TRUE(applyAllReplacements(Replaces, Context.Rewrite));
+  EXPECT_FALSE(Context.Rewrite.overwriteChangedFiles());
+  EXPECT_EQ("line1\nreplaced\nline3\nline4",
+                 getFileContentFromDisk("input.cpp"));
+}
+
 namespace {
 template <typename T>
 class TestVisitor : public clang::RecursiveASTVisitor<T> {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D1800.2.patch
Type: text/x-patch
Size: 4991 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131003/794200f6/attachment.bin>