[PATCH] D102964: [lld-macho] Implement cstring deduplication

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/SyntheticSections.cpp:1081-1082
+      builder(StringTableBuilder::RAW,
+              /*Alignment=*/target->cpuType == CPU_TYPE_X86_64 ? 16 : 1) {
+  align = target->cpuType == CPU_TYPE_X86_64 ? 16 : 1;
+  flags = S_CSTRING_LITERALS;
----------------
gkm wrote:
> The extremity of the target-dependent difference in alignment requirement is surprising, and worthy of a comment.
Good point. I've copied the relevant bits of the commit message.


================
Comment at: lld/test/MachO/cstring-merging.s:54-65
+_local_foo1:
+  .asciz "foo"
+_local_foo2:
+  .asciz "foo"
+L_.foo1:
+  .asciz "foo"
+L_.foo2:
----------------
gkm wrote:
> Is there value in testing ...
> * Strings of length other than 3?
> * Zero length?
> * Non-null terminated?
> * Prefix matches? (e.g. "foo" and "fool", or "bar" and "barf")
yeah I got lazy here... those are good suggestions. I don't think prefix matches are necessary since we are no longer doing tail merging, but the rest seem useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102964