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

[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;
----------------
grimar wrote:
> 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).
Ah, I missed that. I agree with keeping this NFC.


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

https://reviews.llvm.org/D66517