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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Two comment wrapping issues are still outstanding (I've pinged them).

In https://reviews.llvm.org/D42516#1021711, @jakehehrlich wrote:

> It's not an ideal test but to test that


I think you forgot to end a sentence :) Also I don't see any comment about number 6?



================
Comment at: test/tools/llvm-objcopy/many-sections.test:41
+SECS: Name: .symtab_shndx
+SECS-NEXT: Type: SHT_SYMTAB_SHND
+SECS-NEXT: Flags [ (0x0)
----------------
SHT_SYMTAB_SHNDX?


================
Comment at: test/tools/llvm-objcopy/many-sections.test:46
+SECS-NEXT: Offset:
+SECS-NEXT: Size:
+SECS-NEXT: Link: 65285
----------------
I think checking the size here would be wise. It should be easy to do, since you know how many symbols there are. A comment describing the calculation would be helpful too, i.e. something like "# Size == 4 * symbol count".


================
Comment at: tools/llvm-objcopy/Object.cpp:875
+    // If the section name string table section index is greater than or equal
+    // to
+    // SHN_LORESERVE (0xff00), this member has the value SHN_XINDEX (0xffff) and
----------------
jhenderson wrote:
> Weird wrapping again?
Ping


================
Comment at: tools/llvm-objcopy/Object.cpp:1131
+  if (NeedsLargeIndexes) {
+    // This means we definitely need to have a section index table but if
+    // we already have one then we should use it instead of making a new
----------------
jhenderson wrote:
> Have you wrapped slightly early here? I think we can get this comment down to 2 lines.
Ping


Repository:
  rL LLVM

https://reviews.llvm.org/D42516