[PATCH] D89891: [llvm-ar][Object] Fix detection of need for 64-bit archive symbol tables

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:294
+  assert((OffsetSize == 4 || OffsetSize == 8) && "Unsupported OffsetSize");
+  uint64_t Size = OffsetSize; // Number of entries
   if (isBSDLike(Kind))
----------------
`Size` is a size of the symbol table, not a number of entries, right? Though I am not sure it makes much sense to have a comment about it, I think it is clear from the function name?


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:337
+
+  uint64_t OffsetSize = is64BitKind(Kind) ? sizeof(uint64_t) : sizeof(uint32_t);
+  uint32_t Pad;
----------------
Perhaps just `is64BitKind(Kind) ? 8 : 4`? (it is a very often used construction in LLVM).


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:610
+    raw_svector_ostream SymtabHeader(SymtabHeaderBuf);
+    writeSymbolTableHeader(SymtabHeader, Kind, Deterministic, SymtabSize);
+
----------------
I was a bit confused when saw `writeSymbolTableHeader` here.
`Kind` can be changed below:

```
    if (LastOffset >= (1ULL << Sym64Threshold)) {
      if (Kind == object::Archive::K_DARWIN)
        Kind = object::Archive::K_DARWIN64;
      else
        Kind = object::Archive::K_GNU64;
    }
```

I think would be more natural to call `writeSymbolTableHeader` after last possible modification of `Kind`, i.e. after this code block. Is it possible?


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

https://reviews.llvm.org/D89891