[PATCH] D65189: [MC] Support returning a structured rich disassembly output

[Seiya Nuta via Phabricator via llvm-commits]

seiya added inline comments.
Herald added a subscriber: wuzish.


================
Comment at: llvm/include/llvm/MC/MCInstPrinter.h:79
+  size_t StartOffset;
+  // A stack of pointers which points to SpansOut_ or InnerSpans of unclosed
+  // Spans.
----------------
jhenderson wrote:
> This isn't a stack, it's a vector. Use the correct terminology for the type.
> 
> Could it be a vector of ArrayRefs?
> This isn't a stack, it's a vector. Use the correct terminology for the type.
Here I use std::vector as stack. I'd like to use std::stack instead but it does not provide `clear` method. I've updated the comment to stress my intention.

> Could it be a vector of ArrayRefs?
No. We need a pointer to a vector to append elements into the vector.


================
Comment at: llvm/include/llvm/MC/MCInstPrinter.h:162-163
+
+  // Resets the MarkupState. This should be called first in printInst().
+  void resetMarkup(raw_ostream &OS);
+
----------------
jhenderson wrote:
> This feels like bad code design to me, since it would be incredibly easy for a use to forget to do this. How about creating printInst and printInstImpl functions. The former calls resetMarkup and then printInstImpl. The latter is the pure virtual method that sub-classes implement.
I'm totally agree with you and adding printInstImpl sounds good idea. Thanks! 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65189