[PATCH] D104699: [llvm-objdump] Print comments for the disassembled code

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added a subscriber: sdesmalen.
ikudrin added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AArch64/disassemble-print-comments.s:7
+# CHECK:      0000000000000000 <foo>:
+# CHECK-NEXT:        0:   add x0, x2, #2, lsl #12    // =8192
+# CHECK-NEXT:        4:   add z31.d, z31.d, #65280   // =0xff00
----------------
MaskRay wrote:
> For `lsl #12` it is useful to display 8192.
> 
> But for `add z31.d, z31.d, #65280`, 0xff00 may be less useful. GNU objdump displays many such immediates in hexadecimal, if I switch it to hexadecimal, then it should be clear the comment will not be needed.
I guess that a person who added generating the comments, @sdesmalen, found them useful for whatever reason. For my personal taste, it is better to have more comments than fewer. They can always be easily ignored if you do not need them.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1042
 
+static void emitEOLStuff(formatted_raw_ostream &FOS, const MCAsmInfo &MAI,
+                         const MCSubtargetInfo &STI, StringRef Comments,
----------------
jhenderson wrote:
> "Stuff" sounds a little unprofessional, although I am struggling to come up with a good alternative!
> 
> Maybe `emitPostInstructionInfo`?
Thanks for the suggestion!


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1427
+
+          IP->setCommentStream(llvm::nulls());
 
----------------
jhenderson wrote:
> I'm not really up-to-speed on a lot of the MC stuff, but is there a need to do this?
`CommentStream` is local to the function while `IP` is external, thus the provided reference should be removed. We could set the comment stream somewhere at the top of the function, where it is created, and remove it at the end, but the function is very long and these phases would be too far away from each other. Such code would be a bit fragile to my taste and could be broken unintentionally during development.


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

https://reviews.llvm.org/D104699