[PATCH] D33892: Align definition of DW_OP_plus with DWARF spec [1/3]

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Thanks! I noticed that in the remainder of the patch there are still references to DW_OP_plus left, but they are completely untested. I would recommend removing all referenced to DW_OP_plus in this patch and then adding support for the "real" DW_OP_plus back in a separate commit, together with testcases.



================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:555
+    case 1:
+      // Move DW_OP_deref to the end.
+      if (N && Expr[0] == dwarf::DW_OP_deref) {
----------------
remove extra newline :-)


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:575
+        switch (SubExpr.front()) {
+        default:
+          break;
----------------
For readability, let's move the `HistoricSize = 1` initialization here.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:590
+        HistoricSize = std::min(SubExpr.size(), HistoricSize);
+        ArrayRef<uint64_t> Args = SubExpr.slice(1, HistoricSize-1);
+
----------------
This could be expressed more compactly as:
```
auto Op = SubExpr.front();
Buffer.push_back(Op == DW_OP_plus ? dwarf::DW_OP_plus_uconst : Op);
Buffer.append(Args.begin(), Args.end());
```


================
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:325
     }
     case dwarf::DW_OP_plus:
+    case dwarf::DW_OP_plus_uconst:
----------------
Either we handle DW_OP_plus correctly here, or it should be removed.


================
Comment at: lib/IR/DebugInfoMetadata.cpp:601
   case dwarf::DW_OP_constu:
   case dwarf::DW_OP_plus:
   case dwarf::DW_OP_minus:
----------------
This must be removed.


================
Comment at: lib/IR/DebugInfoMetadata.cpp:646
+    case dwarf::DW_OP_plus_uconst:
     case dwarf::DW_OP_plus:
     case dwarf::DW_OP_minus:
----------------
This must be removed.


https://reviews.llvm.org/D33892