[PATCH] D37735: [ELF] - Remove one of OutputSectionFactory::addInputSec().

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/OutputSections.cpp:208-209
 
-void OutputSectionFactory::addInputSec(InputSectionBase *IS,
-                                       StringRef OutsecName) {
-  // Sections with the SHT_GROUP attribute reach here only when the - r option
-  // is given. Such sections define "section groups", and InputFiles.cpp has
-  // dedup'ed section groups by their signatures. For the -r, we want to pass
-  // through all SHT_GROUP sections without merging them because merging them
-  // creates broken section contents.
-  if (IS->Type == SHT_GROUP) {
-    OutputSection *Out = nullptr;
-    addInputSec(IS, OutsecName, Out);
-    return;
-  }
-
-  // Imagine .zed : { *(.foo) *(.bar) } script. Both foo and bar may have
-  // relocation sections .rela.foo and .rela.bar for example. Most tools do
-  // not allow multiple REL[A] sections for output section. Hence we
-  // should combine these relocation sections into single output.
-  // We skip synthetic sections because it can be .rela.dyn/.rela.plt or any
-  // other REL[A] sections created by linker itself.
-  if (!isa<SyntheticSection>(IS) &&
-      (IS->Type == SHT_REL || IS->Type == SHT_RELA)) {
-    auto *Sec = cast<InputSection>(IS);
-    OutputSection *Out = Sec->getRelocatedSection()->getOutputSection();
-    addInputSec(IS, OutsecName, Out->RelocationSection);
-    return;
-  }
-
-  SectionKey Key = createKey(IS, OutsecName);
-  OutputSection *&Sec = Map[Key];
-  addInputSec(IS, OutsecName, Sec);
-}
-
-void OutputSectionFactory::addInputSec(InputSectionBase *IS,
-                                       StringRef OutsecName,
-                                       OutputSection *&Sec) {
-  if (!IS->Live) {
-    reportDiscarded(IS);
-    return;
-  }
-
+static void addSection(InputSectionBase *IS, StringRef OutsecName,
+                       OutputSection *&Sec) {
   if (Sec && Sec->Live) {
----------------
grimar wrote:
> ruiu wrote:
> > grimar wrote:
> > > ruiu wrote:
> > > > Instead of mutating a given pointer through a reference, this function should take `OutputSection *` and returns `OutputSection *`.
> > > Ok, but personally I find such constructions a bit odd. 
> > Really? Then that's pretty different from my taste. Mutating an argument pointer through a reference is pretty tricky, and on the other hand, returning a pointer is very normal, straightforward and functional-ish.
> Returning pointer itself - yes, but not in constructions when it is on both sides like
> `Sec = addSection(IS, OutsecName, Sec);` 
> Mutating pointer is also not great though.
IMO there's a big difference. The expression `Sec = addSection(IS, OutsecName, Sec)` is easier to understand because within the narrow local context, we know that Sec is a reference to a pointer, so what that expression does is obvious: Sec is updated. On the other hand, if you implicitly mutate Sec through a reference, that is not obvious at all.

You cannot assume that people who are reading this piece of code already know what exactly `addSection` does, so you want to make it easy to guess. One way of doing it is to make something explicitly. Generally, you want to avoid mutating variables implicitly through references unless there's other reason to do that.

In addition to that, the previous code

  OutputSection *Out = nullptr;
  addSection(IS, OutsecName, Out);

was ugly. I was tempted to remove `Out` variable, but I couldn't because it has to be an lvalue.

Overall, I strongly believe that changing this function to return a pointer instead of mutating an argument via a reference is pretty much reasonable change which improves code quality.


Repository:
  rL LLVM

https://reviews.llvm.org/D37735