[PATCH] [lld] enable mach-o and native yaml to be intermixed

[kledzik at apple.com]

================
Comment at: include/lld/ReaderWriter/Reader.h:25
@@ +24,3 @@
+namespace llvm {
+  namespace yaml {
+    class IO;
----------------
Rui Ueyama wrote:
> Don't indent namespaces.
The coding guide says to indent namespaces if they are small.

================
Comment at: lib/ReaderWriter/MachO/File.h:24
@@ -23,2 +23,3 @@
 
-  void addDefinedAtom(StringRef name, ArrayRef<uint8_t> content) {
+  void addDefinedAtom(StringRef name, ArrayRef<uint8_t> content, bool copyRefs){
+    if (copyRefs) {
----------------
Rui Ueyama wrote:
> nit: need a space before "{"
But that would push the line of 80 columns.  This seems more readable than moving the parameter to a new line.

================
Comment at: lib/ReaderWriter/MachO/File.h:29
@@ +28,3 @@
+      memcpy(s, name.data(), name.size());
+      name = StringRef(s, name.size());
+      uint8_t *bytes = _allocator.Allocate<uint8_t>(content.size());
----------------
Rui Ueyama wrote:
> Does name = *new (_allocator) std::string(name.str()) work?
It would be nice to have a utility to do this in one step, but I don't think that std:string expression will work.  First, it assumes the string is '\0' terminated.  Next, the std::string will be allocation using _allocator, but if the string allocates a buffer for its copy, that buffer will be made using the default allocator.  

================
Comment at: lib/ReaderWriter/YAML/ReaderWriterYAML.cpp:671
@@ +670,3 @@
+    if (info->_registry && info->_registry->handleTaggedDoc(io, file))
+        return;
+    // If no registered handler claims this tag and there is no tag, 
----------------
Rui Ueyama wrote:
> Wrong indentation.
fixed

================
Comment at: lib/ReaderWriter/MachO/MachONormalizedFileYAML.cpp:640
@@ +639,3 @@
+    if (foe) {
+      std::unique_ptr<lld::File> f = std::move(*foe);
+      file = f.release();
----------------
Rui Ueyama wrote:
> Rafael recently added get method to ErrorOr, which I think is preferred way to get a value of ErrorOr, so please use it instead of *.
Yes.  That is nicer, especially when there is a combined ErrorOr<> and std::unique_ptr<>.  Fixed.

================
Comment at: lib/ReaderWriter/MachO/MachONormalizedFileYAML.cpp:628
@@ +627,3 @@
+                                                 const lld::File *&file) const {
+  if (io.mapTag("!mach-o")) {
+    // Step 1: parse yaml into normalized mach-o struct.
----------------
Rui Ueyama wrote:
> I'd invert the expression: if (!io.mapTag("!mach-o")) return false; for early return.
Done.

================
Comment at: include/lld/ReaderWriter/YamlContext.h:38
@@ -37,5 +37,3 @@
 
-  const LinkingContext *_linkingContext;
-  const Registry *_registry;
-  File *_file;
-  NormalizedFile *_normalizeMachOFile;
+  const LinkingContext   *_linkingContext;
+  const Registry         *_registry;
----------------
Joey Gouly wrote:
> I wish we could stick with the original code for this. It's what clang-format produces, and while it might be a little harder to skim down and see the types and names of class members, I think it's worth the consistent style.
> 
> Nick, I know you like this style, but are you willing to try convert to clang-format style? (You don't even have to format your code, it's brilliant!)
I think of clang-format as a great tool for the first-pass, bulk cleanup of code.  But there are still lots of areas where a human would format it differently to be even more readable.  That is why I don't like to run clang-format on code that is already hand tuned, because it removes lots of the hand improvements.  I'll write some bugs on clang-format for some of the cases I care about and see what it can do.


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

BRANCH
  svn

ARCANIST PROJECT
  lld