[PATCH] D42516: [llvm-objcopy] Add support for large indexes

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:811
+    // """
+    // If the number of sections is greater than or equal to
+    // SHN_LORESERVE (0xff00), this member has the value zero and the actual
----------------
jhenderson wrote:
> Comment looks like it's wrapped early?
I want "SHN_LORESERVE (0xff00)" to stay together which means I have to wrap early. It looksreally odd if you wrap the parenthsies to the next line. Since this is a direct quote I don't want to take the "(0xff00)" out to better resolve that issue.


================
Comment at: tools/llvm-objcopy/Object.cpp:854-859
+  // """
+  // If the number of sections is greater than or equal to
+  // SHN_LORESERVE (0xff00), this member has the value zero and the actual
+  // number of section header table entries is contained in the sh_size field
+  // of the section header at index 0.
+  // """
----------------
jhenderson wrote:
> I feel like having these quotes duplicated is not great. I wonder whether it's a sign that the null shdr should be constructed in the writeEhdr function, so that the decisions only need making the once? Thoughts?
I'm a bit conflicted on this one. I like having the logic for writing the whole section header table in one place because it makes the logic of "do we write the section header table out or not" more succinct. I also however agree with your claim that quoting this twice is annoying.

I can think of two options here:
1) remove the comments in the second case and just leave a URL
2) move this to writeEhdr just like you said


Repository:
  rL LLVM

https://reviews.llvm.org/D42516