[PATCH] D53905: [ELF] Refactor TLS layout TargetInfo config. NFC.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/InputSection.cpp:572-573
+  // target-specific adjustment to produce a thread-pointer-relative offset.
+  switch (Target->TlsLayout) {
+    case TlsLayoutKind::FixedOffset:
+      return Target->TlsTpOffset;
----------------
jrtc27 wrote:
> ruiu wrote:
> > I don't think you need to define TlsLayoutKind. I'd just dispatch based on `Config->EMachine` in this function.
> That works, and there are certainly other parts of LLD that do things like that, but you could say the same thing about `TlsTpOffset` and `TcbSize` themselves... I personally like to see `Config->EMachine` used as little as possible and put the logic in `ELF/Arch/$ARCH.cpp`, but of course as maintainer you ultimately decide where to draw the line for when to abstract, and it's not exactly a big deal (though does then disregard the original motivation for this patch).
> 
> Also, on a separate issue, the case labels (and bodies) should be deindented one level to be flush with the `switch`.
`TlsTpOffset` and `TcbSize` are just member variables that doesn't have any logic, but `TlsLayoutKind` is a new thing with which we compute some value, and we use that member variable only in this function. So I think eliminating `TlsLayoutKind` and directly use `Config->EMachine` matches the taste of lld's code. I do understand your motivation to write target-dependent code in files under `Arch/`, but we are not too serious about doing that. We have target-dependent code in many other places if it makes code easier to read.

As to the indentation depth, `case` should be at the same nesting level as `switch` in the LLVM coding style.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D53905