[PATCH] D56587: Introduce DW_OP_LLVM_convert

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

We're almost there!



================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:512
+
+/// EmitValue - Emit base type reference.
+///
----------------
aprantl wrote:
> under the current style you may drop the duplicate doxygen comments con the function implementations.
please remove this comment from the implementation.


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:521
+/// SizeOf - Determine size of the base type reference in bytes.
+///
+unsigned DIEBaseTypeRef::SizeOf(const AsmPrinter *AP, dwarf::Form Form) const {
----------------
ditto.. it should be on the declaration in the header file.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1934
+
+  // XXX: The expressions are inserted into a byte stream rather early (see
+  // DwarfExpression::addExpression) so for those ops (e.g. DW_OP_convert)
----------------
Remove the first word.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1947
+  uint32_t Offset = 0;
+  for (auto &Op : Expr) {
+    Streamer.EmitInt8(Op.getCode(), Comment != End ? *(Comment++) : "");
----------------
Could you please still factor this into a static fixupBaseTypeRefs(or something along those lines) function for better readability?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1950
+    Offset++;
+    for (unsigned I = 0; I < 2; ++I) {
+      if (Op.getDescription().Op[I] == Encoding::SizeNA)
----------------
Please add an assert that fails if the opcode is `DW_OP_const_type` as it is not supported by this loop.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1952
+      if (Op.getDescription().Op[I] == Encoding::SizeNA)
+        continue;
+      if (Op.getDescription().Op[I] == Encoding::BaseTypeRef) {
----------------
Where is this getting copied?


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

https://reviews.llvm.org/D56587