This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Refactor lazy symbol duplicated code.
ClosedPublic

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

Details

Summary

Our code for LazyObject and LazyArchive duplicates.

This patch extracts the common part to remove
the duplication.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Apr 11 2018, 5:51 AM
espindola added inline comments.Apr 11 2018, 5:18 PM
ELF/SymbolTable.cpp
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?

ELF/SymbolTable.h
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
ELF/SymbolTable.cpp
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)

and

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())
  Symtab->addFile<ELFT>(File);

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.