[PATCH] D53690: [llvm-objdump] Don't Crash When Using `-a` on Non-Archives (PR39402)

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objdump/non-archive-object.test:3-4
+# RUN: llvm-objdump --archive-headers %t > %t.out1
+# RUN: llvm-objdump -a %t > %t.out2
+# RUN: cmp %t.out1 %t.out2
+# RUN: FileCheck %s --input-file=%t.out1
----------------
I don't think you need to worry about testing both switches in this test, as this is testing a specific aspect of the switch, rather than the generic behaviour. That will then allow you to write the llvm-objdump and FileCheck on the same line:

`# RUN: llvm-objdump --archive-headers %t | FileCheck %s`


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:2309
 
   if (ArchiveHeaders && !MachOOpt)
+    printArchiveChild(ArchiveName, c);
----------------
Higuoxing wrote:
> eush wrote:
> > Higuoxing wrote:
> > > eush wrote:
> > > > It seems like you could just add `&& c` here and in the second overload and the signature of `printArchiveChild` could stay the same.
> > > Thanks a lot, but I think checking `nullptr` inside `printArchiveChild` will be more readable ?
> > I disagree. `DumpArchive`, `DumpObject`, as well as other functions in this file are never called on `nullptr`, the pattern is to check first, then call. Why should `printArchiveChild` work differently?
> > 
> > Also, from code-complexity point of view, this patch introduces one new conditional statement where there could be none.
> Sorry, I misunderstand your opinion. I just want to make a safe `printArchiveName`, it seems that not a good idea to introduce a new conditional statement ... thanks a lot
Personally, I'm a big fan of being explicit in my checks against null, i.e. write `c != nullptr` instead of just `c`, but I'm not too bothered either way.


https://reviews.llvm.org/D53690