Page MenuHomePhabricator

[yaml2obj] - Make interface of `NameToIdxMap` class be human friendly and fix users.
ClosedPublic

Authored by grimar on Apr 26 2019, 8:10 AM.

Details

Summary

This patch inverses the values returned by addName and
lookup methods of the class mentioned so that they
now return true on success and false on failure.
Also, it does minor code cleanup.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Apr 26 2019, 8:10 AM

I'm happy with this in principle, but I do wonder how you made sure you caught all instances!

tools/yaml2obj/yaml2elf.cpp
78 ↗(On Diff #196854)

As you're tidying this comment up, please change "asserts" to "Asserts".

400 ↗(On Diff #196854)

What happens if you specify a section name that doesn't exist? Does this result in an assertion? If so, we should produce an actual error instead in this case.

Similar point may apply below.

grimar marked an inline comment as done.Apr 30 2019, 9:47 AM

I do wonder how you made sure you caught all instances!

I am usually using renaming magic for such things. I.e. I renamed addName to addName1, lookup to lookup1 and so on,
then tried to compile and fixed all the places. Then renamed back.

tools/yaml2obj/yaml2elf.cpp
400 ↗(On Diff #196854)

Oh, I did not notice we do not fail if list a section that do not exist. I think we should check it earlier than here then.
What about landing the following refactoring then? D61322

grimar updated this revision to Diff 197668.May 1 2019, 4:21 PM
grimar marked an inline comment as done.
grimar removed a reviewer: ruiu.
  • Rebased.
tools/yaml2obj/yaml2elf.cpp
78 ↗(On Diff #196854)

Done. And for Returns above too.

jhenderson accepted this revision.May 2 2019, 1:47 AM

LGTM.

tools/yaml2obj/yaml2elf.cpp
78 ↗(On Diff #196854)

Not sure if those are necessary or not due to doxygen!

This revision is now accepted and ready to land.May 2 2019, 1:47 AM
grimar edited the summary of this revision. (Show Details)May 2 2019, 12:25 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2019, 12:27 PM