This is an archive of the discontinued LLVM Phabricator instance.

[ObjectYAML][MachO] Add error handling in MachOEmitter.

Authored by Higuoxing on May 25 2020, 10:20 PM.



Currently, yaml2macho doesn't support error handling. This patch helps improve it.

Diff Detail

Event Timeline

Higuoxing created this revision.May 25 2020, 10:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2020, 10:20 PM
jhenderson added inline comments.May 26 2020, 2:23 AM

Perhaps invalid_argument instead of inconvertibleErrorCode()?


Here and elsewhere, I don't think you need an assertion if you are reporting an error too.


I think these two test cases should each be in their own file, rather than a generic "errors" test file.


Perhaps worth having one entry here, to show that zero entries is not specifically the issue here.


nit: not sure if this function is really that necessary, it feels like createStringError is already concise enough.
p.s. , this probably should be 'static' instead.


+ 1

Higuoxing updated this revision to Diff 266394.May 26 2020, 7:16 PM

Addressed comments.

  • Use errc::invalid_argument instead of inconvertibleErrorCode()
  • Remove assertions
  • Move test cases to their own test files.
  • Modify the Fat MachO test case according to the comment.
Higuoxing updated this revision to Diff 266395.May 26 2020, 7:23 PM

Address @alexshap 's comment.

  • Use createStringError()
jhenderson accepted this revision.May 27 2020, 2:03 AM

Some comment nits, otherwise LGTM. Please wait for @alexshap to confirm too.

1 ↗(On Diff #266395)

generating the -> generating

I think canonically it should be "Mach-O" rather than "MachO" here and elsewhere in comments.

93 ↗(On Diff #266395)

'##' for comments.

1 ↗(On Diff #266395)

"in the MachO object file" -> "in Mach-O object files" I think reads better.

335 ↗(On Diff #266395)

the -> an

This revision is now accepted and ready to land.May 27 2020, 2:03 AM
Higuoxing updated this revision to Diff 266546.May 27 2020, 7:57 AM

Address comments.

This revision was automatically updated to reflect the committed changes.