This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Move getSymbols() methods to InputFile.
ClosedPublic

Authored by grimar on Jul 27 2017, 4:36 AM.

Details

Summary

I suggest to do this change, together with D35987 it allows to detemplate code like:

for (elf::ObjectFile<ELFT> *File : Symtab<ELFT>::X->getObjectFiles()) {
  for (SymbolBody *Body : File->getSymbols()) {
    auto *D = dyn_cast<DefinedRegular>(Body);
    if (!D || !D->Section)
      continue;

Going to use it in D35843, which need iterating over DefinedRegular symbols
from non-ELFT-templated code.

Patch detemplates finalizeShtGroup() to show it is itself can be useful for detemplation.

Diff Detail

Event Timeline

grimar created this revision.Jul 27 2017, 4:36 AM
ruiu added inline comments.Jul 31 2017, 9:01 PM
ELF/InputFiles.h
88

Move the definition to the .cpp file.

88

Do not rename this method unless it is needed. getSymbols should just work.

91

return {}

grimar updated this revision to Diff 109071.Aug 1 2017, 3:48 AM
grimar edited the summary of this revision. (Show Details)
  • All comments addressed.
ruiu edited edge metadata.Aug 1 2017, 6:29 AM

Overall, it doesn't seem like an improvement. You don't need to push too hard to remove templates. Use of templates is not necessarily bad.

That being said, if you think that this change could remove a lot of uses of templates, please do that in this patch to demonstrate it.

grimar updated this revision to Diff 109509.Aug 3 2017, 3:27 AM
grimar retitled this revision from [ELF] - Move getSymbols() method to InputFile. to [ELF] - Move getSymbols() methods to InputFile..
  • Moved all getSymbols() methods to InputFile.
ruiu added inline comments.Aug 3 2017, 4:47 AM
ELF/OutputSections.cpp
431

This change is dubious and shouldn't be here.

grimar updated this revision to Diff 109574.Aug 3 2017, 9:10 AM
  • Addressed review comment.
ELF/OutputSections.cpp
431

Fixed.

ruiu accepted this revision.Aug 4 2017, 2:31 AM

LGTM

ELF/InputFiles.h
87

on files not of ... types -> on files of other types.

as what "other types" means is obvious.

This revision is now accepted and ready to land.Aug 4 2017, 2:31 AM
This revision was automatically updated to reflect the committed changes.