This is an archive of the discontinued LLVM Phabricator instance.

[NFC][llvm-readobj] Pull common code into a helper
ClosedPublic

Authored by jhenderson on Oct 31 2019, 9:51 AM.

Details

Summary

This will make planned changes to this code easier to make.

Diff Detail

Event Timeline

jhenderson created this revision.Oct 31 2019, 9:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2019, 9:51 AM
Herald added a subscriber: seiya. · View Herald Transcript
MaskRay accepted this revision.Oct 31 2019, 10:15 AM

Nice!

This revision is now accepted and ready to land.Oct 31 2019, 10:15 AM
grimar accepted this revision.Nov 1 2019, 1:13 AM

LGTM. Thoughs about possible futher refactoring is inline.

llvm/tools/llvm-readobj/ELFDumper.cpp
5445

A side note: I think we should probably get rid of First and pass the symbols index instead.
Seems First is used only for that purpose, but it is probably a bit confusing to see 2 Elf_Sym arguments
in a functions called like printSymbolSection, i.e. its name kind of assumes a single symbol, though arguments list has 2.

grimar added inline comments.Nov 1 2019, 1:15 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
5448

To make the change above, this method and methods inside should be changed.

jhenderson marked an inline comment as done.Nov 1 2019, 3:03 AM
jhenderson added inline comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
5448

I'm changing the getSectionNameIndex method in D69670 to split it up. Is that what you're referring to? Once that change lands, it might be possible to change the Object library getExtendedSymbolTableIndex interface to take an index, or I could change the new getSymbolSectionIndex function to figure out the value of First instead of passing it in.

grimar added inline comments.Nov 1 2019, 3:10 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
5448

I'm changing the getSectionNameIndex method in D69670 to split it up. Is that what you're referring to?

No, I did not see D69670 when reviewed this one, but anyways I meant this and getExtendedSymbolTableIndex and may be something else will need a change to take an index if we deside to get rid of 'First'.

I think you can land your set of patches first and we can refactor this piece after that, as it is independent and unrelated actually.

This revision was automatically updated to reflect the committed changes.