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

[Rui Ueyama]

================
Comment at: include/lld/ReaderWriter/Simple.h:72
@@ -71,1 +71,3 @@
 
+class FileToMutable : public SimpleFile {
+public:
----------------
Shankar Kalpathi Easwaran wrote:
> Rui Ueyama wrote:
> > 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.
> There is no way that you could replace atoms. The FileToMutable class keeps a reference to the old file where the atoms did belong. 
You do not actually use the reference to the old file, but copies all the atoms from the old file to "this" file in the constructor.

I did not say that we should replace the atoms of existing MutableFile. What I said is that this doesn't have to be a new class, but can be an independent function. In that function, you can create a new SimpleFile and add all atoms from old file to the SimpleFile by calling addAtom(). This would be cleaner because (1) it uses only public interface, so it's clear that the method does not depend on any internal data structure of SimpleFile, and (2) can keep private fields you made protected private.

================
Comment at: lib/Passes/RoundTripNativePass.cpp:43
@@ +42,3 @@
+
+  llvm::sys::fs::remove(tmpNativeFile.str());
+}
----------------
Shankar Kalpathi Easwaran wrote:
> Rui Ueyama wrote:
> > Instead of removing the file at the end of the function, you should use llvm::FileRemover for RAII-style temporary file management.
> No we shouldnt be using FileRemover here, as if there is a problem with the native file or the yaml file, it can be seperately debugged very easily.
I'm not sure if that's the right thing to do to retain a temporary file when a test fails. But if you want it, I think we should print out the file name when it fails. It's also nice to leave a comment that says we intentionally do *not* remove a file when something goes wrong.


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