[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/lib/MC/MCParser/AsmParser.cpp:3882
+  CVDefRangeType CVDRType = (CVTypeIt == CVDefRangeTypeMap.end())
+                                ? CVDR_DEFRANGE
+                                : CVTypeIt->getValue();
----------------
nilanjana_basu wrote:
> rnk wrote:
> > 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.
> This is a placeholder enum, like DK_NO_DIRECTIVE in DirectiveKind. If the defrange type is not found, it is set to this value, & its executed in the default case. I have only implemented the 4 types that are implemented in CodeViewDebug.cpp.
I saw that you left the asm printer method that prints the binary string version of .cv_defrange, so I thought maybe you wanted to keep support for that and handle it that way here. We can either completely remove support for the string variant of the directive, or add support for it here. Either way is fine.


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:3890
+        parseAbsoluteExpression(DRRegister))
+      return Error(Loc, "expected register number");
+
----------------
I realize you should write tests for the error handling paths. You can add a cv-defrange-errors.s test file that exercises each of these corner cases, and then FileCheck for the errors.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65179