Page MenuHomePhabricator

[llvm-objdump] - Remove one overload of reportError. NFCI.
ClosedPublic

Authored by grimar on Aug 21 2019, 2:00 AM.

Details

Summary

There is a problem with reportError we have. Declaration says we have ArchiveName
that follows the FileName:

reportError(Error E, StringRef FileName, StringRef ArchiveName,...

Though implementation have them reversed. I cleaned it up and
removed an excessive reportError(Error E, StringRef File) version.

Rebased on top of D66418.

Diff Detail

Event Timeline

grimar created this revision.Aug 21 2019, 2:00 AM

No test cases changed, so it is kind of NFCI.

MaskRay accepted this revision.Aug 21 2019, 2:33 AM
This revision is now accepted and ready to land.Aug 21 2019, 2:33 AM
jhenderson added inline comments.Aug 21 2019, 2:33 AM
tools/llvm-objdump/MachODump.cpp
2409 ↗(On Diff #216346)

Should this be:

reportError(std::move(E), Filename, "", ArchitectureName);

Remove one of the reportError functions.

"Remove one overload of reportError" is conciser

grimar retitled this revision from [llvm-objdump] - Remove one of the `reportError` functions. to [llvm-objdump] - Remove one overload of reportError..Aug 21 2019, 4:08 AM
grimar marked an inline comment as done.Aug 21 2019, 4:25 AM
grimar added inline comments.
tools/llvm-objdump/MachODump.cpp
2409 ↗(On Diff #216346)

I am not familar with MachO, but I think - no. Seems MachOUniversalBinary can contain
multiple objects and also Filename is reported as archive name around this place.

Note that my change did not change the original logic, and also that this error is untested.

MaskRay added inline comments.
tools/llvm-objdump/MachODump.cpp
2409 ↗(On Diff #216346)

I think George's change here is fine. At least it doesn't introduce new problems.

Maybe @seiya can help check whether there is something that should be improved? (@enderby seems to be inactive for a while)

seiya added inline comments.Aug 26 2019, 2:31 AM
tools/llvm-objdump/MachODump.cpp
2409 ↗(On Diff #216346)

I prefer @jhenderson's suggestion. Reporting Filename as an archive name would look a bit weird since the filename is empty:

llvm-objdump: error: foo.fat.invalid(): ...

Note that users should be able to determine which object in the universal binary has a problem from its ArchitectureName (I believe there're no objects with the same cpu arch in an universal binary...).

grimar retitled this revision from [llvm-objdump] - Remove one overload of reportError. to [llvm-objdump] - Remove one overload of reportError. NFCI..Aug 26 2019, 2:43 AM
grimar marked an inline comment as done.
grimar added inline comments.
tools/llvm-objdump/MachODump.cpp
2409 ↗(On Diff #216346)

But my patch is NFC in current state. I'd like to keep it NFC.

Changing that can be done independently (before or after doing this change, and needs addind a test case then).

seiya added inline comments.Aug 26 2019, 3:12 AM
tools/llvm-objdump/MachODump.cpp
2409 ↗(On Diff #216346)

Ah, I missed that. I agree with keeping this NFC.

jhenderson accepted this revision.Aug 27 2019, 2:05 AM

LGTM.

tools/llvm-objdump/MachODump.cpp
2409 ↗(On Diff #216346)

Okay, seems reasonable to me.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2019, 3:13 AM