[PATCH] D60376: [llvm-objdump] Align instructions to a tab stop

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objdump/X86/disassemble-align.s:4
+
+# -w/--wide is ignored. llvm-objdump always produces wide output.
+# RUN: llvm-objdump -wd -print-imm-hex %t | sed 'y/\t/ /' | FileCheck -strict-whitespace %s
----------------
MaskRay wrote:
> jhenderson wrote:
> > Since you did -w/--wide in a different change, you should probably remove the references to it from here.
> This is to demonstrate that our output format is similar to that of `objdump -w`. I want to add a test for `--wide` `-w` and I think here is probably the best place. I'd be happy to move it to a difference place if you have a better suggestion.
> This is to demonstrate that our output format is similar to that of `objdump -w`
I don't think this is something we should be testing in this sense. This test looks to be to do with alignment of columns.

What might make sense is to have a separate test that shows that the default, with --wide, and with -w all produce identical output (since --wide is a silent no-op). This should be a separate change, and should have been part of the change that added --wide.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:609
+
+    {
+      formatted_raw_ostream FOS(OS);
----------------
MaskRay wrote:
> jhenderson wrote:
> > Why do you need to scope this block?
> Let `~formatted_raw_ostream` call `flush()` before we call `IP.printInst(MI, OS, "", STI);`
> 
> This is required if `outs()` is buffered (e.g. when redirecting to a file)
Okay. This probably deserves a comment then.

Alternatively, is it possible to just pass `FOS` into the `printInst` call?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60376