[PATCH] D87497: [llvm-objcopy][MachO] Fix --add-section

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

The functional changes are probably too far outside my expertees to be able to sensibly review. As for the test change, I'm not convinced by the amount of duplication in the test. I feel like if something needed to change slightly in the objcopy algorithm again and/or the test input, the duplication of the test checks could just lead to missing a potential update required (which in turn might mask a bug). If I follow it correctly, the key difference in each of the cases is the offset value? If that's the case, have you considered using FileCheck numeric variables (which have been added since I think this test was originally written), to avoid having to hard-code the offset for each case? I don't know if they're powerful enough in their current form, and I also don't know what information you'd need to rely on to calculate the required values, so this might not be viable.



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/add-section.test:266
+# CASE3-32BIT-NEXT:    )
+
----------------
Nit: too many blank lines at EOF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87497