This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Consistently use createStringError instead of make_error<StringError>. NFC.
ClosedPublic

Authored by mstorsjo on Jan 21 2019, 4:44 AM.

Diff Detail

Event Timeline

mstorsjo created this revision.Jan 21 2019, 4:44 AM
jhenderson accepted this revision.Jan 21 2019, 5:18 AM

Make sure to mention in the commit comment that this also quotes certain symbol names.

I'm surprised that several of the error messages appear to be untested (only one test needed updating, but there are at least 3 different behaviour changes here). I take it testing them is not straightforward?

LGTM anyway.

This revision is now accepted and ready to land.Jan 21 2019, 5:18 AM

Make sure to mention in the commit comment that this also quotes certain symbol names.

Ah, right, yes - will do.

I'm surprised that several of the error messages appear to be untested (only one test needed updating, but there are at least 3 different behaviour changes here). I take it testing them is not straightforward?

Correct, the two cases where I add quotes but there's no test change for are currently not possible to trigger, I think - both the input as set by Reader, and updates by Object currently should make them impossible to trigger. I guess asserts would be an option then as well. (At an earlier point in the evolution of the removeSections patch, some of them were triggerable, but I realized it's easy to do the right thing instead of erroring out on dangling associative sections). There's also lots of error messages in Reader that aren't tested, mainly for broken inputs.

Correct, the two cases where I add quotes but there's no test change for are currently not possible to trigger, I think - both the input as set by Reader, and updates by Object currently should make them impossible to trigger. I guess asserts would be an option then as well. (At an earlier point in the evolution of the removeSections patch, some of them were triggerable, but I realized it's easy to do the right thing instead of erroring out on dangling associative sections). There's also lots of error messages in Reader that aren't tested, mainly for broken inputs.

Okay, it might be nice to turn the unreachable ones into asserts (or simply delete the code), but that's not relevant to this change.

This revision was automatically updated to reflect the committed changes.