[PATCH] D65190: [X86] X86ATTInstPrinter: replace markup with startMarkup/endMarkup

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp:41-42
+
+// Since printRegName is a const method, we need this non-const version to
+// update MarkupSpans.
+void X86ATTInstPrinter::printMarkedUpRegName(raw_ostream &OS, unsigned RegNo) {
----------------
seiya wrote:
> rupprecht wrote:
> > What if instead of adding this method, you make the MarkupSpans fields `mutable` so startMarkup/endMarkup can be marked const? This would enable markup support more broadly for other tools that call this method.
> > Alternatively, my other suggestion of replacing endMarkup() w/ a destructor on MarkupStart would allow MarkupStart itself to own the MarkupSpans, making it naturally const.
> > 
> > (I think this is fine to commit as-is and address in a later patch)
> > What if instead of adding this method, you make the MarkupSpans fields mutable so startMarkup/endMarkup can be marked const?
> I've suggested marking it `mutable` in an earlier comment, but @jhenderson prefers keeping `printMarkedUpRegName` instead. `printRegName` is the only method which needs to be const so this is not so bad change I think. I want to remove it if possible, btw.
> 
> > Alternatively, my other suggestion of replacing endMarkup() w/ a destructor on MarkupStart would allow MarkupStart itself to own the MarkupSpans, making it naturally const.
> I suppose that it can't be const: after all, it appends a new MarkupSpan into the output vector (which set by  `setMarkupSpans` in D65189).
> 
> 
FWIW, I don't feel strongly about the use of `mutable`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65190