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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:536
 
+  void upgradeMetadataExpression(uint64_t FromVersion,
+                                 MutableArrayRef<uint64_t> Expr,
----------------
Let's rename this to `upgradeDIExpression`.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:536
 
+  void upgradeMetadataExpression(uint64_t FromVersion,
+                                 MutableArrayRef<uint64_t> Expr,
----------------
Also, `FromVersion` is unused.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:550
+      case dwarf::DW_OP_LLVM_fragment:
+        HistoricSize = 3;
+        break;
----------------
you could `break` here, since DW_OP_LLVM_fragment must be the last operator in the expression.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:559
+
+      switch(SubExpr.front()) {
+      case dwarf::DW_OP_plus:
----------------
clang-format please


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:568
+
+      // Continue with remaining elements
+      SubExpr = SubExpr.slice(HistoricSize);
----------------
LLVM coding standard wants a `.` at the end of each comment.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:569
+      // Continue with remaining elements
+      SubExpr = SubExpr.slice(HistoricSize);
+    }
----------------
Copying the expression here seems to be unnecessary, but I assume the reason is that the DW_OP_minus upgrade cannot be done in place.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:1612
     case 2:
+      upgradeMetadataExpression(Version, Elts, UpgradedExpr);
+      Elts = MutableArrayRef<uint64_t>(UpgradedExpr);
----------------
I would move the entire switch statement into the upgradeMetadataExpression helper. 


https://reviews.llvm.org/D33892