Page MenuHomePhabricator

[ELF] - Refactor lazy symbol duplicated code.

Authored by grimar on Apr 11 2018, 5:51 AM.



Our code for LazyObject and LazyArchive duplicates.

This patch extracts the common part to remove
the duplication.

Diff Detail


Event Timeline

grimar created this revision.Apr 11 2018, 5:51 AM
espindola added inline comments.Apr 11 2018, 5:18 PM
601 ↗(On Diff #141984)

You can move this definition to the only use:

replaceOrIgnoreLazy(Name, [&]....)

623 ↗(On Diff #141984)

Can this be a static helper?

101 ↗(On Diff #141984)

Add a comment saying what replaceOrIgnoreLazy does.

ruiu added a comment.Apr 11 2018, 5:30 PM

Isn't it more complicated than before? I understand that it is tempting, but generally speaking, I try to *not* abstract things unless doing it is a clear win. I'm a bit worried the direction of your recent "refactoring" patches as it adds more abstractions for a (in my opinion) marginal benefits. Factoring out common code is not always a win in terms of readability, especially when it involves callback functions and such. Simple, repeated code is sometimes better, and the original code was written with that in mind. I really prefer boring code over clever code that uses higher order functions.

grimar updated this revision to Diff 142145.Apr 12 2018, 4:12 AM
  • Reimplemented, addressed comments.

What do you think about this version, Rui? Is it still too much?

espindola added inline comments.Apr 16 2018, 9:28 AM
608 ↗(On Diff #142145)

If you change the argument order in the constructors to:

LazyArchive(InputFile &File, uint8_t Type,
            const llvm::object::Archive::Symbol S)


LazyObject(InputFile &File, uint8_t Type, StringRef Name)

then it is fairly easy to make replaceOrFetchLazy a variadic template in the same way that replaceSymbol is. That would save one callback argument.

621 ↗(On Diff #142145)

The implementations of Fetch are not using S. Also, if you template this over ELFT you can write

if (InputFile *File = Fetch())

And the lambdas are a lot simpler.

grimar updated this revision to Diff 143550.Apr 23 2018, 7:06 AM
  • Updated the patch basing on Rafael's comments.
This revision is now accepted and ready to land.Apr 23 2018, 8:30 PM
This revision was automatically updated to reflect the committed changes.