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

[Seiya Nuta via Phabricator via llvm-commits]

seiya 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;
----------------
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...).



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

https://reviews.llvm.org/D66517