[PATCH] D87656: [llvm-dwarfdump] --show-sources option to show all sources

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/sources.test:348
+# RUN:   -filetype=obj -o %t.malformed.o
+# RUN: not llvm-dwarfdump --show-sources %t.malformed.o 2> %t.malformed.err | \
+# RUN:   count 0
----------------
Usually when we're just checking an error or warning message, we use `2>&1` to combine stderr and stdout, rather than checking the two separately. This is especially relevant here, because you do the `--implicit-check-not` check.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/sources.test:1-3
+# RUN: yaml2obj --docnum=1 %s -o - \
+# RUN: | llvm-dwarfdump --show-sources - \
+# RUN: | FileCheck --check-prefix=CU-NAME-CHECK %s
----------------
mysterymath wrote:
> jhenderson wrote:
> > jhenderson wrote:
> > > Up to you, but I have a personal preference for this formatting, as it indicates on each line that there is a continuation involved, starting with a new command.
> > > 
> > > Also, I personally prefer it if tests create objects on disk rather than passing them via stdin. This is because it's easier to directly inspect the binary if there's a problem with the test. Otherwise, you have to (temporarily) modify the test to force it to dump it.
> > > 
> > > Same comments apply elsewhere.
> > You missed the space indentation I included in my suggested edit.
> Not sure what you mean here; I'm seeing three space indentation from the colon in `RUN:` in both the code and your suggested edit.
Sorry misread the test before (I thought FileCheck was still being passed input by pipe, which it wasn't in that iteration). Latest bit looks good.


================
Comment at: llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp:482-483
+  llvm::Optional<uint64_t> LastIndex = LT.getLastValidFileIndex();
+  for (uint64_t I = LT.hasFileAtIndex(0) ? 0 : 1,
+                E = LastIndex ? *LastIndex + 1 : 0;
+       I != E; ++I) {
----------------
jhenderson wrote:
> I'm not sure I see testing that exercises both sides of each of the two ternaries in this loop.
Did this get addressed (it might have done, but I don't have time to dig into the test coverage)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87656