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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/MC/MCInstPrinter.h:43
+
+// MarkupSpan represents a marked up range in the disassembly. For example:
+//
----------------
Doxygen style comments please, to match the rest of the file.


================
Comment at: llvm/include/llvm/MC/MCInstPrinter.h:63
+  // InnerSpans contains one MarkupSpan which represents `<reg:%rip>`.
+  std::unique_ptr<std::vector<MarkupSpan>> InnerSpans;
+
----------------
Why is this a unique_ptr? Can't you just have a std::vector here directly?


================
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.
----------------
This isn't a stack, it's a vector. Use the correct terminology for the type.

Could it be a vector of ArrayRefs?


================
Comment at: llvm/include/llvm/MC/MCInstPrinter.h:82
+  // TODO: Rename to a appropriate name.
+  std::vector<std::vector<MarkupSpan> *> SpansVectors;
+
----------------
Why not just Spans?


================
Comment at: llvm/include/llvm/MC/MCInstPrinter.h:84-85
+
+  void reset(raw_ostream &OS, bool Enabled_,
+             std::vector<MarkupSpan> *SpansOut_);
+  size_t offset(raw_ostream &OS) const;
----------------
Don't use names with trailing underscores. Just do NewEnabled and NewSpans or whatever.


================
Comment at: llvm/include/llvm/MC/MCInstPrinter.h:159
 
+  // Specify an output vector of marked up ranges.
+  void setMarkupSpans(std::vector<MarkupSpan> &MS) { MarkupSpans = &MS; }
----------------
I'm not sure this comment makes sense. Also, the comments here should be in doxygen style, to match existing members.

Perhaps "Set the vector to write marked up ranges to" would be better phrasing.


================
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);
+
----------------
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.


================
Comment at: llvm/lib/MC/MCInstPrinter.cpp:166-167
+
+    // TODO: support tag-modifier-list. As far as I investigated, it is not
+    // used though. See: https://llvm.org/docs/MarkedUpDisassembly.html
+    size_t Length = 2 + TypeStr.size();
----------------
I'd probably just delete the second sentence here.


================
Comment at: llvm/lib/MC/MCInstPrinter.cpp:172
+
+      /* we'll set Length and InnerLength later. */
+      MarkupSpan Span = MarkupSpan(M.Type, M.State.offset(OS), 0,
----------------
Use C++ style comments, and start the comment with an upper-case letter. It would be better to write this in the passive too: "Length and InnerLength will be set later."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65189