This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by grimar on Sep 12 2017, 3:01 AM.

Details

Summary

Patch removes one of OutputSectionFactory::addInputSec methods.
That allows to simplify reporting of discarded sections and
should help to D37561.

Diff Detail

Event Timeline

grimar created this revision.Sep 12 2017, 3:01 AM
ruiu edited edge metadata.Sep 12 2017, 5:07 PM

The new code is hard to understand due to a pointer to a pointer to something. I think you can do this without two levels of pointer indirection.

grimar updated this revision to Diff 115017.Sep 13 2017, 5:32 AM
grimar retitled this revision from [ELF] - Combine two OutputSectionFactory::addInputSec(). to [ELF] - Remove one of OutputSectionFactory::addInputSec()..
grimar edited the summary of this revision. (Show Details)

After playing with code I "re-invented" nicest way: just leave everything as is and make one of addInputSec
to be static helper. That way public addInputSec becomes the only way to add input section and it is able to report
discarded sections early enough, what solves initial problem.

ruiu added inline comments.Sep 14 2017, 11:15 AM
ELF/OutputSections.cpp
208–210

Instead of mutating a given pointer through a reference, this function should take OutputSection * and returns OutputSection *.

grimar updated this revision to Diff 115383.Sep 15 2017, 2:42 AM
  • Addressed review comments.
grimar updated this revision to Diff 115384.Sep 15 2017, 2:46 AM
  • Addressed comments.
ELF/OutputSections.cpp
208–210

Ok, but personally I find such constructions a bit odd.

ruiu accepted this revision.Sep 15 2017, 8:02 AM

LGTM

ELF/OutputSections.cpp
208–210

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.

This revision is now accepted and ready to land.Sep 15 2017, 8:02 AM
grimar added inline comments.Sep 15 2017, 8:42 AM
ELF/OutputSections.cpp
208–210

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.

This revision was automatically updated to reflect the committed changes.
ruiu added inline comments.Sep 15 2017, 9:04 AM
ELF/OutputSections.cpp
208–210

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.

grimar added inline comments.Sep 18 2017, 8:17 AM
ELF/OutputSections.cpp
208–210

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