Page MenuHomePhabricator

[Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 4]
AcceptedPublic

Authored by abrachet on Aug 10 2019, 11:09 PM.

Details

Summary

Added addSection and addSymbol methods

Diff Detail

Event Timeline

abrachet created this revision.Aug 10 2019, 11:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2019, 11:09 PM

I think some other interesting test cases are a) adding more than one symbol/section, b) adding and then removing a symbol/section, c) removing stuff and then adding stuff, showing that the iteration still works correctly.

llvm/include/llvm/Object/MutableELFObject.h
172

My instinct is that this should be a void return type.

340

I think this should just be a void return type.

345

I think this should just be a void return type.

393–398

This seems like an unrelated change?

llvm/unittests/Object/MutableELFObjectTest.cpp
568–570

Comment why there's 4 sections already.

589–591

Comment why there's already a symbol here.

593

It's not a header. Symbols are just that - symbols (unlike sections which have a header and actual contents). So this should just be called "Sym" or something like that.

abrachet updated this revision to Diff 215331.Aug 14 2019, 11:44 PM

Addressed review comments

abrachet marked 5 inline comments as done.Aug 14 2019, 11:49 PM
abrachet added inline comments.
llvm/include/llvm/Object/MutableELFObject.h
172

Hmm, I'm not sure about this and the other comments. I think it is more convenient this way, but also I wonder when you would add something and then immediately change something. The cost of this return is very small though I would say.

I don't have a strong opinion either way.

jhenderson added inline comments.Aug 15 2019, 8:23 AM
llvm/include/llvm/Object/MutableELFObject.h
172

My thinking is that add is akin to vector's push_back method, which doesn't return anything.

llvm/unittests/Object/MutableELFObjectTest.cpp
568

"yaml2obj creates the .symtab ..."

590

"... creates the index 0 symbol even when ..."

636

for (size_t I = 0; I < 10000; ++I) would seem more natural.

I'd also reduce it to 1000 or 100 for speed.

647

AddRemoveSymbol

682

AddRemoveSection

711–728

Could you add some new lines in this block between the groups of setup followed by checks, to make it easier to see where the interesting tests actually are, please.

abrachet updated this revision to Diff 215521.Aug 15 2019, 6:54 PM

Addressed review comments

abrachet marked 10 inline comments as done.Aug 15 2019, 6:54 PM
abrachet updated this revision to Diff 215536.Aug 15 2019, 8:26 PM

Fixed failing test for std::distance call.

jhenderson accepted this revision.Mon, Aug 19, 1:53 AM

LGTM, though @rupprecht should confirm too.

This revision is now accepted and ready to land.Mon, Aug 19, 1:53 AM
MaskRay added inline comments.
llvm/include/llvm/Object/MutableELFObject.h
340

void addSymbol(MutableELFSymbol<ELFT> Sym)

If you don't have a lvalue overload, taking an rvalue is usually inferior to pass-by-value because you'll not able to call addSymbol with a lvalue.

343

void addSection(MutableELFSection<ELFT> Sym)

I think the method is self-explanatory. No need for a comment.

MaskRay added inline comments.Mon, Aug 19, 10:54 PM
llvm/include/llvm/Object/MutableELFObject.h
42

Omit Other, but why do you need the defaulted move ctor?

174

Swap the two statements to omit - 1.

abrachet updated this revision to Diff 216324.Tue, Aug 20, 8:24 PM

Addressed comments. Made unit tests better.

abrachet marked 4 inline comments as done.Tue, Aug 20, 8:25 PM
jhenderson accepted this revision.Wed, Aug 21, 2:00 AM

Latest changes LGTM.

rupprecht accepted this revision.Wed, Aug 21, 11:32 AM