[PATCH] D99633: [lld-macho][nfc] Refactor in preparation for 32-bit support

[Jez Ng via Phabricator via llvm-commits]

int3 marked an inline comment as done.
int3 added a comment.

> But target->wordSize also goes through the vtable?

I don't think that's true...? Why would it?

> And you are loosing section_64 and have to use the less type-safe offset.

I don't see how we can avoid this without templatizing TargetInfo.... inheritance / virtual dispatch doesn't get around the problem of having to handle both `section` and `section_64`. (And FWIW, LLD-ELF passes offsets to TargetInfo too, presumably for the same reason)



================
Comment at: lld/MachO/Arch/ARM64.cpp:31
 
-  int64_t getEmbeddedAddend(MemoryBufferRef, const section_64 &,
+  int64_t getEmbeddedAddend(MemoryBufferRef, uint64_t offset,
                             const relocation_info) const override;
----------------
oontvoo wrote:
> seems inconsistent that this parameter has name and the  other two don't
My philosophy is to avoid redundancy :) `MemoryBufferRef mbref` is saying the same thing twice, whereas `offset` actually provides information that `uint64_t` doesn't


================
Comment at: lld/MachO/Arch/ARM64.cpp:49-50
+enum : uint64_t {
+  // Using this instead of target->wordSize allows the compiler to better
+  // optimize.
+  WordSize = 8,
----------------
oontvoo wrote:
> Interesting. How so? (In the previous version I saw you were using `uint64_t WordSize = ...`, which makes more sense)
> Can you explain why using an anonymous enum here is better than that?
lint doesn't approve of `uint64_t WordSize` (it wants `wordSize`). That would result in it being shadowed by `TargetInfo::wordSize`. I think this is less ugly than having to use `::wordSize`...

Optimization-wise, I think this is the same as using `constexpr` -- at least, the compiler is able to use the constant value as an immediate in both cases


================
Comment at: lld/MachO/Target.h:89
+
+struct LP32 {
+  using mach_header = llvm::MachO::mach_header;
----------------
oontvoo wrote:
> not to be pedantic, but maybe `ILP32` ?
for sure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99633