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

[Rui Ueyama via llvm-commits]

On Mon, Sep 18, 2017 at 8:17 AM, George Rimar via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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).


You need to make your code easy to read without knowing the entire picture
of your code. Implicitly mutating arguments via references makes your code
hard to understand because it requires readers to understand not only the
caller but the callee function. So, please avoid that whenever possible.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170927/b85ac64f/attachment.html>