This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

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

Diff Detail

Unit TestsFailed

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
llvm/lib/ObjectYAML/MachOEmitter.cpp
31

Perhaps invalid_argument instead of inconvertibleErrorCode()?

287

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

llvm/test/ObjectYAML/MachO/errors.yaml
1 ↗(On Diff #266105)

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

15 ↗(On Diff #266105)

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

llvm/lib/ObjectYAML/MachOEmitter.cpp
31

nit: not sure if this function is really that necessary, it feels like createStringError is already concise enough.
p.s. https://llvm.org/docs/CodingStandards.html#anonymous-namespaces , this probably should be 'static' instead.

287

+ 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.

llvm/test/ObjectYAML/MachO/fat_macho_i386_x86_64.yaml
1–6

generating the -> generating

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

93

'##' for comments.

llvm/test/ObjectYAML/MachO/sections.yaml
1–5

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

335

"##"
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.