[PATCH] D77893: [lld] Merge Mach-O input sections

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: lld/MachO/OutputSection.h:41
+  // Some sections may allow coalescing other raw input sections.
+  virtual void mergeInput(InputSection *input);
+
----------------
Ktwu wrote:
> smeenai wrote:
> > Ktwu wrote:
> > > smeenai wrote:
> > > > Is this needed for anything other than MergedOutputSection?
> > > No, it's not needed except for MergedOutputSection. Should I dynamically cast each output section before trying to merge an input section into it instead (I wanted to avoid the runtime hit doing that).
> > Yeah, dynamically casting wouldn't be ideal. We could leave this as-is, or make `getOrCreateOutputSection` return a `MergedOutputSection *` instead of an `OutputSection *` (idk if that'd cause other complications).
> I believe that would require having an assert that the output class being returned, if already created, is in fact a mergeable section. I think a static_cast would be what we'd want, but I like having the explicit assert here in case something goes wrong instead of the undefined behavior of a static_cast gone wrong.
Ah. The assert would require an `isa`, which is basically the same overhead as a `dyn_cast`, so perhaps just leaving it as-is is best for now.


================
Comment at: lld/MachO/Writer.cpp:328-330
+    getOrCreateOutputSegment(isec->segname)
+        ->getOrCreateOutputSection(isec->name)
+        ->mergeInput(isec);
----------------
Ktwu wrote:
> smeenai wrote:
> > Nit: might be nicer to assign these to variables instead of chaining.
> I personally prefer chaining :D
Haha, fair enough.


================
Comment at: lld/MachO/Writer.cpp:376
   // dyld requires __LINKEDIT segment to always exist (even if empty).
-  getOrCreateOutputSegment(segment_names::linkEdit);
-  // No more segments can be created after this method runs.
+  auto *linkEditSegment = getOrCreateOutputSegment(segment_names::linkEdit);
+
----------------
Ktwu wrote:
> smeenai wrote:
> > Nit: use an explicit type instead of auto
> What's up with `auto`? It's not like it's forbidden from use in the style guide:
> 
> https://llvm.org/docs/CodingStandards.html#id27
> 
> I'm curious about folks' reasoning behind its usage (or discouragement of).
Good question. LLD has its own style guidelines (e.g. variables are lowerCamelCase). Unfortunately, I don't think those are codified anywhere, but I'm basing this on what I've seen in reviews.

`auto` is discouraged unless the actual type is spelled out in the same expression somewhere (e.g. as a result of a cast) or is a huge pain to type out (e.g. iterators). Otherwise explicit types are preferred. I personally like that because I don't use an IDE (and I haven't set up any ctags or LSP-like things for my editor), so having explicit types available makes it easier for me to comprehend the code.

It's a little less clear-cut in cases like

```
auto linkEditSegment = getOrCreateOutputSegment(segment_names::linkEdit);
```

where it'd be pretty fair to assume that `getOrCreateOutputSegment` returns an `OutputSegment *`. It's a bit ambiguous whether it'd be a pointer, reference, or copy, but you could use `auto *` to disambiguate that. Nevertheless, it's not too much more typing to just spell the name out, so I'd prefer to err on the side of explicitness there.

There was a post to the mailing list about auto usage in LLVM a while back, but there was no clear resolution: http://llvm.1065342.n5.nabble.com/llvm-dev-RFC-Modernizing-our-use-of-auto-td123947.html (the authors on that thread are bogus; if you want the original authors, look for that subject on http://lists.llvm.org/pipermail/llvm-dev/2018-November/ and http://lists.llvm.org/pipermail/llvm-dev/2018-December/)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77893