This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj] Move redundant statements into a separate static function
ClosedPublic

Authored by Higuoxing on Dec 3 2018, 7:28 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

Higuoxing created this revision.Dec 3 2018, 7:28 AM
jhenderson requested changes to this revision.Dec 4 2018, 1:43 AM
jhenderson added inline comments.
tools/yaml2obj/yaml2elf.cpp
229 ↗(On Diff #176402)

"check" implies to me that it's just validating something. However, this function does more than that (it sets the Index variable too). I'd suggest renaming the function convertSectionIndex or similar.

230 ↗(On Diff #176402)

SecField to me implies the name of a section field, e.g. "sh_link". I'd suggest renaming SecField to IndexSrc or similar, and Index to IndexDest. The two are closely related, so having similar names makes sense to me.

278 ↗(On Diff #176402)

This isn't a section index lookup. It's a symbol lookup. Note that the message is different.

This revision now requires changes to proceed.Dec 4 2018, 1:43 AM
jhenderson added inline comments.Dec 4 2018, 1:44 AM
tools/yaml2obj/yaml2elf.cpp
278 ↗(On Diff #176402)

By the way, were there no test failures because of this? If there were none, I'd suggest adding a test for this particular case, again in a separate change.

Higuoxing marked 2 inline comments as done.Dec 4 2018, 2:19 AM
Higuoxing added inline comments.
tools/yaml2obj/yaml2elf.cpp
278 ↗(On Diff #176402)

Sure, I'll add one.

Higuoxing marked an inline comment as done.Dec 4 2018, 2:20 AM
Higuoxing added inline comments.
tools/yaml2obj/yaml2elf.cpp
278 ↗(On Diff #176402)

I noticed this problem, because they are all of type NameToIdxMap.
I did not noticed the error hints are different, I'll fix them, thanks for your reviewing

Higuoxing updated this revision to Diff 176579.Dec 4 2018, 2:37 AM
Higuoxing updated this revision to Diff 176583.Dec 4 2018, 2:52 AM
jhenderson added inline comments.Dec 4 2018, 4:45 AM
tools/yaml2obj/yaml2elf.cpp
239 ↗(On Diff #176583)

As this is only used the once, I don't think that it is useful to pull this case out into a separate function. I could be persuaded otherwise though.

Higuoxing updated this revision to Diff 176617.EditedDec 4 2018, 6:16 AM
Higuoxing marked an inline comment as done.

I just want to make the logic inside initSectionHeaders simple. I could wait until there are two places that needs simplify ...

Thank you @jhenderson, for your kind reviewing :)

This revision is now accepted and ready to land.Dec 4 2018, 6:28 AM
This revision was automatically updated to reflect the committed changes.