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

Rui Ueyama ruiu at google.com
Mon Oct 28 22:01:37 PDT 2013



================
Comment at: include/lld/ReaderWriter/Simple.h:72
@@ -71,1 +71,3 @@
 
+class FileToMutable : public SimpleFile {
+public:
----------------
Shankar Kalpathi Easwaran wrote:
> 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
It looks like the private field _file is not really used by anyone, so I don't understand the first paragraph. I'd think we can s/_file/file/ the constructor without any changes in meaining. The field is not referenced by no one else, and is not used for resource management.

This is not a super important in this code review, but it looks like you misunderstood my point, so let me describe it again. What I suggest is make a function like this:

  SimpleFile *fileToMutable(const LinkingContext &context, File &file) {
    SimpleFile *ret = new SimpleFile(context, file.path());
    for (auto atom : file.defined())
      ret.addAtom(std::move(atom));
    // do the same thing for undefined, shlib and absolute
    return ret;
  }

In this approach you don't have to create a new class (simple class hierarchy is generally a good thing :). You also don't have to make private fields protected.


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



More information about the llvm-commits mailing list