[PATCH] D115007: Dump swift5 reflection section data into dsym bundle generated binary with dsymutil

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:299
 
+static bool binaryHasSwiftReflectionSections(const DebugMap &Map,
+                                             LinkOptions &Options,
----------------
Instead of having an in-our argument, could this function return an `Expected<bool>` instead? I also find `NoError` confusing because you end up checking a double negation, i.e. `!NoError`. 


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:300
+static bool binaryHasSwiftReflectionSections(const DebugMap &Map,
+                                             LinkOptions &Options,
+                                             BinaryHolder &BinHolder,
----------------
Why are the LinkOptions not `const`? 


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:348-351
+    const std::unique_ptr<llvm::dsymutil::DebugMapObject,
+                          std::default_delete<llvm::dsymutil::DebugMapObject>>
+        &Obj,
+    std::unique_ptr<DwarfStreamer> &Streamer) {
----------------
You' assuming that these pointers are non-null so why not pass `DebugMapObject` and `Streamer` by references? If that's not an option, I'd just pass the underlying pointers instead. I can't think of any benefit of passing the unique_ptr by reference instead.


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:357-358
+  } else {
+    auto *MO = dyn_cast<llvm::object::MachOObjectFile>(OF->getBinary());
+    if (MO) {
+      for (auto &Section : OF->getBinary()->sections()) {
----------------
A common style in LLVM is to do `if(Foo* foo = ...) {`. 


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:366
+        }
+        auto SectionContents = Section.getContents();
+        if (SectionContents) {
----------------
This use of `auto` doesn't seem obvious to me (https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable)


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:486-488
+    if (!ReflectionSectionsPresentInBinary) {
+      copySwiftReflectionMetadata(Obj, Streamer);
+    }
----------------
No braces around single-line if's (https://llvm.org/docs/CodingStandards.html#id59)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115007/new/

https://reviews.llvm.org/D115007