[PATCH] [PassManager] add calls to RoundTrip{YAML, Native} Reader/Writer

[Rui Ueyama]

================
Comment at: include/lld/Core/PassManager.h:35
@@ -34,3 +34,3 @@
 
-  ErrorOr<void> runOnFile(MutableFile &);
+  ErrorOr<void> runOnFile(std::unique_ptr<MutableFile> &);
 
----------------
We don't need to do that in this patch, but ErrorOr<void> looks a bit odd, because it returns nothing  other than an error code. I'd make it to return an llvm::error_code.

================
Comment at: include/lld/Core/Resolver.h:7
@@ -6,2 +6,3 @@
 // License. See LICENSE.TXT for details.
+// License. See LICENSE.TXT for details.
 //
----------------
duplicate?

================
Comment at: include/lld/Core/Resolver.h:67
@@ -64,5 +66,3 @@
 
-  MutableFile& resultFile() {
-    return _result;
-  }
+  std::unique_ptr<MutableFile> resultFile() { return std::move(_result); }
 
----------------
To me it does not make sense to release the ownership of _result when resultFile() is called. We can make the LinkingContext object to keep having the ownership. Another merit of not releasing the ownership would be that you wouldn't have to change the function signatures in many places (from MutableFile& to std::unique_ptr<MutableFile>.)

================
Comment at: include/lld/Passes/RoundTripNativePass.h:25
@@ +24,3 @@
+
+  /// Sorts atoms in mergedFile by content type then by command line order.
+  virtual void perform(std::unique_ptr<MutableFile> &mergedFile);
----------------
Need to update the comment.

================
Comment at: include/lld/Passes/RoundTripNativePass.h:10
@@ +9,3 @@
+
+#ifndef LLD_PASSES_ROUNDTRIP_NATIVE_PASS_H
+#define LLD_PASSES_ROUNDTRIP_NATIVE_PASS_H
----------------
A rule of the include guard seems inserting an underscore before a capital letter in camel case identifier. So we should s/ROUNDTRIP/ROUND_TRIP/.

================
Comment at: include/lld/Passes/RoundTripYAMLPass.h:10
@@ +9,3 @@
+
+#ifndef LLD_PASSES_ROUNDTRIP_YAML_PASS_H
+#define LLD_PASSES_ROUNDTRIP_YAML_PASS_H
----------------
Ditto

================
Comment at: include/lld/Passes/RoundTripYAMLPass.h:1
@@ +1,2 @@
+//===- Passes/RoundTripYAMLPass.h- Write YAML file/Read it back-----------===//
+//
----------------
nit: need space before "-"

================
Comment at: include/lld/Passes/RoundTripYAMLPass.h:25
@@ +24,3 @@
+
+  /// Sorts atoms in mergedFile by content type then by command line order.
+  virtual void perform(std::unique_ptr<MutableFile> &mergedFile);
----------------
Update the comment.

================
Comment at: include/lld/ReaderWriter/Simple.h:72
@@ -71,1 +71,3 @@
 
+class FileToMutable : public SimpleFile {
+public:
----------------
I guess you added this class to convert a file a mutable file object. I'd think we shouldn't define a new class for that. This can be implemented as a function that is not a member of any class that will create a new instance of SimpleFile from a given File object.

================
Comment at: lib/Passes/RoundTripNativePass.cpp:2
@@ +1,3 @@
+//===- Passes/RoundTripNativePass.cpp - Layout atoms
+//-------------------------------===//
+//
----------------
Line wrapping

================
Comment at: lib/Passes/RoundTripYAMLPass.cpp:2
@@ +1,3 @@
+//===- Passes/RoundTripYAMLPass.cpp - Layout atoms
+//-------------------------------===//
+//
----------------
Line wrapping

================
Comment at: lib/Passes/RoundTripNativePass.cpp:10
@@ +9,3 @@
+//===----------------------------------------------------------------------===//
+//===----------------------------------------------------------------------===//
+
----------------
duplicate

================
Comment at: lib/Passes/RoundTripYAMLPass.cpp:10
@@ +9,3 @@
+//===----------------------------------------------------------------------===//
+//===----------------------------------------------------------------------===//
+
----------------
duplicate

================
Comment at: lib/Passes/RoundTripNativePass.cpp:43
@@ +42,3 @@
+
+  llvm::sys::fs::remove(tmpNativeFile.str());
+}
----------------
Instead of removing the file at the end of the function, you should use llvm::FileRemover for RAII-style temporary file management.

================
Comment at: lib/Passes/RoundTripYAMLPass.cpp:43
@@ +42,3 @@
+
+  llvm::sys::fs::remove(tmpYAMLFile.str());
+}
----------------
Ditto


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