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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen marked an inline comment as done.
sdesmalen added inline comments.


================
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) {
----------------
aprantl wrote:
> remove extra newline :-)
Perhaps Phabricator doesn't display it properly, but I don't see an extra newline here.
I did remove the extra newline between 'auto N = ..' and the switch statement.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:590
+        HistoricSize = std::min(SubExpr.size(), HistoricSize);
+        ArrayRef<uint64_t> Args = SubExpr.slice(1, HistoricSize-1);
+
----------------
aprantl wrote:
> 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());
> ```
Now that I have reshuffled the functionality within patches, I kept the less compact notation because the code also supports DW_OP_minus, which needs to be distinguished as a separate case.


https://reviews.llvm.org/D33892