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

[George Rimar via Phabricator via llvm-commits]

grimar 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) {
----------------
ruiu wrote:
> 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.
> 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. 
> 

Issue is here for me. Such expression does not give guarantee that `Sec` is updated (I mean not variable itself, but instance). It can return new updated `Sec` but also may return the same one from right part unchanged. What is worse it may update Sec from argument and return different `Sec` at the same time. 
In compare with simple assignment like `Sec = addSection(IS, OutsecName)` there is huge difference for me.
(I am not trying to say it is worse than mutating argument, just trying to explain why it confuses me when I see such things and why I don't prefer them).


Repository:
  rL LLVM

https://reviews.llvm.org/D37735