[PATCH] D65179: Changing representation of .cv_def_range directives in Codeview debug info assembly format for better readability

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/include/llvm/MC/MCStreamer.h:38
 
+using namespace llvm::codeview;
+
----------------
Please avoid `using namespace` in header files, since it will affect everyone who includes the header.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1399
     StringRef FixedSizePortion) {
   OS << "\t.cv_def_range\t";
   for (std::pair<const MCSymbol *, const MCSymbol *> Range : Ranges) {
----------------
I suppose this can be factored out as well.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1415-1421
+  OS << "\t.cv_def_range\t";
+  for (std::pair<const MCSymbol *, const MCSymbol *> Range : Ranges) {
+    OS << ' ';
+    Range.first->print(OS, MAI);
+    OS << ' ';
+    Range.second->print(OS, MAI);
+  }
----------------
Please factor this repeated code into a helper method.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1423
+  OS << ", reg_rel, ";
+  OS << DRHdr.Register << ", " << DRHdr.Flags << ", "
+     << DRHdr.BasePointerOffset;
----------------
Ideally, registers should be printed textually as rbp, ebp, etc. Given time constraints, I think we can leave that as a future improvement, though.


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:3882
+  CVDefRangeType CVDRType = (CVTypeIt == CVDefRangeTypeMap.end())
+                                ? CVDR_DEFRANGE
+                                : CVTypeIt->getValue();
----------------
I don't see a case for this enum... does it still work? I have to run to lunch, but I want to send these review comments now rather than later.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65179