[PATCH] D66517: [llvm-objdump] - Remove one overload of reportError. NFCI.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: tools/llvm-objdump/MachODump.cpp:2409
                          ObjOrErr.takeError())) {
-            reportError(std::move(E), Filename, StringRef(), ArchitectureName);
+            reportError(std::move(E), "", Filename, ArchitectureName);
             continue;
----------------
seiya wrote:
> MaskRay wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > Should this be:
> > > > 
> > > > `reportError(std::move(E), Filename, "", ArchitectureName);`
> > > 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.
> > 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)
> 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...).
> 
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).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66517/new/

https://reviews.llvm.org/D66517