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

[Seiya Nuta via Phabricator via llvm-commits]

seiya marked 3 inline comments as done.
seiya added inline comments.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp:379-381
+    O << startMarkup(MarkupType::Imm) << '$';
     Op.getExpr()->print(O, &MAI);
+    O << endMarkup();
----------------
jhenderson wrote:
> No need for this to be part of this change, but I find it slightly odd that you can't use the withMarkup pattern here. I take it that making the print function return a stream wouldn't be straightforward?
> I take it that making the print function return a stream wouldn't be straightforward?
Does "return a stream" mean `raw_ostream &MCExpr::print(raw_ostream &OS, ...);` instead of `void MCExpr::print(...)`?


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp:415-416
       if (ScaleVal != 1) {
-        O << ',' << markup("<imm:") << ScaleVal // never printed in hex.
-          << markup(">");
+        O << ',';
+        withMarkup(O, MarkupType::Imm) << ScaleVal; // never printed in hex.
       }
----------------
jhenderson wrote:
> Could you do the following?
> 
> `O << ',' << withMarkup(O, MarkupType::Imm) << ScaleVal;`
No we can't for now. `llvm::operator<<(raw_ostream &OS, const WithMarkup &M)` is not implemented. I thought it could be confusing to use WithMarkup in such a way because it writes the start tag into the stream in its constructor, not in `operator<<`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65190