[PATCH] cpp11-migrate: Refactor for driver model of operation

[Guillaume Papin]

================
Comment at: cpp11-migrate/Core/FileOverrides.h:91
@@ -210,1 +90,3 @@
+  // \brief Write all file contents overrides to disk.
+  bool write(clang::DiagnosticsEngine &Diagnostics) const;
 
----------------
I think the name can be slightly more explicit but not sure how to name it. `writeToDisk()`, `inPlaceRewrite()`, ...

Maybe a comment about the return value since some functions in Clang returns true on failures (e.g: `Lexer::getRawToken()`) some other false.

================
Comment at: cpp11-migrate/Core/FileOverrides.h:107-112
@@ -223,11 +106,8 @@
 ///
-/// @param SourceFile Full path to the source file.
-/// @param HeaderFile Full path to the header file.
-/// @param Result The resulting unique filename in the same directory as the
-///        header file.
-/// @param Error Description of the error if there is any.
-/// @returns true if succeeded, false otherwise.
-bool generateReplacementsFileName(llvm::StringRef SourceFile,
-                                    llvm::StringRef HeaderFile,
-                                    llvm::SmallVectorImpl<char> &Result,
-                                    llvm::SmallVectorImpl<char> &Error);
+/// @param[in] MainSourceFile Full path to the source file.
+/// @param[out] Result The resulting unique filename in the same directory as
+/// the header file.
+/// @param[out] Error If an error occurs a description of that error is
+/// appended to this string.
+/// @returns true on success, false if a unique file name could not be created.
+bool generateReplacementsFileName(const llvm::StringRef MainSourceFile,
----------------
Maybe take advantage of the documentation changes to convert the `@param` to `\param` to be consistent with the rest of the doxygen comments.

================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:38-53
@@ -165,19 +37,18 @@
 
   // Get the filename portion of the path.
-  llvm::StringRef SourceFileRef(path::filename(SourceFile));
-  llvm::StringRef HeaderFileRef(path::filename(HeaderFile));
+  llvm::StringRef SourceFileRef(path::filename(MainSourceFile));
 
   // Get the actual path for the header file.
-  llvm::SmallString<128> HeaderPath(HeaderFile);
-  path::remove_filename(HeaderPath);
+  llvm::SmallString<128> SourcePath(MainSourceFile);
+  path::remove_filename(SourcePath);
 
   // Build the model of the filename.
   llvm::raw_string_ostream UniqueHeaderNameStream(UniqueHeaderNameModel);
-  UniqueHeaderNameStream << SourceFileRef << "_" << HeaderFileRef
-                         << "_%%_%%_%%_%%_%%_%%" << ".yaml";
-  path::append(HeaderPath, UniqueHeaderNameStream.str());
+  UniqueHeaderNameStream << SourceFileRef << "_%%_%%_%%_%%_%%_%%"
+                         << ".yaml";
+  path::append(SourcePath, UniqueHeaderNameStream.str());
 
   Error.clear();
   if (llvm::error_code EC =
-          fs::createUniqueFile(HeaderPath.c_str(), Result)) {
+          fs::createUniqueFile(SourcePath.c_str(), Result)) {
     Error.append(EC.message().begin(), EC.message().end());
----------------
Maybe unrelated but if we are just appending some text to the file can't we just use: 
```
 fs::createUniqueFile(MainSourceFile + "_%%_%%_%%_%%_%%_%%.yaml", Result))
```


================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:191-195
@@ +190,7 @@
+        Rewrites.getSourceMgr().getFileEntryForID(BufferI->first)->getName();
+    std::string &S = FileStates[FileName];
+    S.clear();
+    llvm::raw_string_ostream StringStream(S);
+    BufferI->second.write(StringStream);
+    StringStream.flush();
+  }
----------------
Alternatively you can write something like:
```
RewriteBuffer &RewriteBuf = BufferI->second;
FileStates[FileName].assign(RewriteBuf.begin(), RewriteBuf.end());
```

================
Comment at: cpp11-migrate/Core/FileOverrides.cpp:205
@@ +204,3 @@
+    std::string ErrorInfo;
+    llvm::raw_fd_ostream FileStream(I->getKey().data(), ErrorInfo);
+    if (!ErrorInfo.empty()) {
----------------
I don't see it written in the StringMap doc that `.data()` is null terminated maybe it's dangerous to rely on that.

Using `Twine::toNullTerminatedStringRef()` can help, see: http://llvm.org/docs/doxygen/html/classllvm_1_1Twine.html#ae1cb2376210907e0aa8a677c98cbb28a

================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:271
@@ +270,3 @@
+                   << "\n";
+      Errors = true;
+    }
----------------
Shouldn't you skip what's next if the filename wasn't generated correctly?


================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:279
@@ +278,3 @@
+      llvm::errs() << "Error opening file: " << ErrorInfo << "\n";
+      Errors = true;
+    }
----------------
Same here, if there was an error opening the file there is no need to try to write the ReplacementsFile.

================
Comment at: cpp11-migrate/tool/Cpp11Migrate.cpp:403-408
@@ +402,8 @@
+    const TUReplacementsMap &ReplacementsMap = T->getAllReplacements();
+    const TranslationUnitReplacements &(
+        TUReplacementsMap::value_type::*getValue)() const =
+        &TUReplacementsMap::value_type::getValue;
+    std::transform(ReplacementsMap.begin(), ReplacementsMap.end(),
+                   std::back_inserter(AllReplacements),
+                   std::mem_fun_ref(getValue));
+
----------------
I feel like it's easier to understand when the member function is used directly where it's necessary. Not sure about the indentation but it seems like it fits 80 columns.

```
    std::transform(ReplacementsMap.begin(), ReplacementsMap.end(),
                   std::back_inserter(AllReplacements),
                   std::mem_fun_ref(&TUReplacementsMap::value_type::getValue));
```

================
Comment at: unittests/cpp11-migrate/FileOverridesTest.cpp:160-164
@@ +159,7 @@
+
+  // SM shouldn't have a FileID for fileB since the SourceManager shouldn't have
+  // seen it yet via overriding.
+  const FileEntry *EntryB = SM.getFileManager().getFile(fileBPath);
+  FileID IdB = SM.translateFile(EntryB);
+  ASSERT_TRUE(IdB.isInvalid());
+}
----------------
This test feels unnecessary to me. The only thing done with `fileBPath` is to map it and I don't see a reason why `applyOverrides()` will modify all of the files mapped in the source manager.


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