[PATCH] [lld] Registry model for Readers and Reference Kind strings

[kledzik at apple.com]

================
Comment at: include/lld/ReaderWriter/CoreLinkingContext.h:32
@@ +31,3 @@
+    TEST_RELOC_LEA32_WAS_GOT = 5,
+  };
+  
----------------
Rui Ueyama wrote:
> Are these fields for testing? If so, is there any way to move this to a unit test file and remove them from this file?
The CoreLinkingContext is used only with "lld -core" which is only used in the test suite.   

Currently, these are only used with the pass-got-basic.objtxt and pass-stubs-basic.objtxt tests.  When I wrote those passes, the intent was that there was a base generic pass, tested by these tests.  And each writer would subclass the generic pass as mach-o does.  But, ELF did not do that, instead it has its own private RelocationPass.  We need to decide it if is worth have base passes and format specific sub-classes (or delegate objects).  If so, we need some sort of generic tests and reference kinds like this.  If not, I can remove all this and push the base classes into mach-o.

================
Comment at: include/lld/ReaderWriter/YamlContext.h:40
@@ +39,3 @@
+  File                  *_file;
+  NormalizedFile        *_normalizeMachOFile;
+};
----------------
Rui Ueyama wrote:
> Why mach-o file only? If we need it, it looks weird that we don't have files for other formats.
We can add ivars for other formats as needed.  This is a step to allowing heterogenous yaml with some files that are the traditional atoms-in-yaml and some that are mach-o-in-yaml.

================
Comment at: lib/Driver/GnuLdDriver.cpp:339
@@ -328,1 +338,3 @@
       }
+      // FIXME: Calling getFileMagic() is is expensive.  It would be better to
+      // wire up the LdScript parser into the registry.
----------------
Rui Ueyama wrote:
> Shankar Kalpathi Easwaran wrote:
> > Agree. Perfect place.
> nit: s/is is/is/
fixed

================
Comment at: lib/Passes/LayoutPass.cpp:73
@@ -72,4 +72,3 @@
     for (const Reference *ref : *atom) {
-      llvm::dbgs() << "    " << ref->kindToString()
-                   << ": " << atomToDebugString(ref->target()) << "\n";
+      llvm::dbgs() << ": " << atomToDebugString(ref->target()) << "\n";
     }
----------------
Rui Ueyama wrote:
> Does it make sense to start a debug output line with ":"?
fixed

================
Comment at: lib/Passes/RoundTripNativePass.cpp:47
@@ +46,3 @@
+  const File *obj = dyn_cast<const File>(objFile);
+  assert(obj && "yaml generated file is not an relocatable file");
+  mergedFile.reset(new FileToMutable(_context, *obj));
----------------
Rui Ueyama wrote:
> s/yaml/native/
Actually, the dyn_cast and assert lines need to be removed.  Fixed.

================
Comment at: lib/ReaderWriter/PECOFF/ReaderImportHeader.cpp:309
@@ +308,3 @@
+        memcmp(buf, "\0\0\xFF\xFF", 4))
+      return make_error_code(NativeReaderError::unknown_file_format);
+
----------------
Rui Ueyama wrote:
> I think you can remove this file magic check as it's already been checked.
Ok.  I moved the size check into canParse() method and got rid of the check here.

================
Comment at: lib/ReaderWriter/Reader.cpp:33
@@ +32,3 @@
+  llvm::sys::fs::file_magic fileType = llvm::sys::fs::identify_magic(content);
+  // Get file extension.
+  StringRef extension = "";
----------------
Rui Ueyama wrote:
> You can use extension(StringRef) declared in llvm/include/llvm/Support/Path.h.
Cool.  Switched to use that.

================
Comment at: lib/ReaderWriter/Reader.cpp:46
@@ +45,3 @@
+    }
+  }
+  
----------------
Rui Ueyama wrote:
> These brackets are not needed.
removed

================
Comment at: lib/ReaderWriter/ELF/Reader.cpp:126
@@ +125,3 @@
+    std::unique_ptr<File> f2(f->release());
+    result.push_back(std::move(f2));
+    return error_code::success();
----------------
Michael Spencer wrote:
> Rui Ueyama wrote:
> > Can we do this as push_back(std::unique_ptr<File>(f->release)), or just push_back(std::move(f))?
> Yes, the previous code should still work.
I reverted to the original.  This was cruft left over from a previous experiment.


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

BRANCH
  svn

ARCANIST PROJECT
  lld