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

[Shankar Kalpathi Easwaran]

================
Comment at: include/lld/ReaderWriter/Simple.h:72
@@ -71,1 +71,3 @@
 
+class FileToMutable : public SimpleFile {
+public:
----------------
Rui Ueyama wrote:
> 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.
The reference to old file gets used as thats still the owner of the atoms, and not the MutableFile. We dont want to have more than one copy of the atom lying around. the FileToMutable class is only a convient class to convert file to a mutable file, and is more cleaner as there is more than one user right now. There might be more users of this class in the future.

Please look at the discussion around this topic in the previous review.

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

================
Comment at: lib/Passes/RoundTripNativePass.cpp:43
@@ +42,3 @@
+
+  llvm::sys::fs::remove(tmpNativeFile.str());
+}
----------------
Rui Ueyama wrote:
> 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.
The passes are inserted only during DEBUG mode. It doesnot happen in release mode. I like the idea that you mentioned to have a comment in the code associated with this. Thanks for the suggestion. I didnt even know about the FileRemover class. 


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