[PATCH] D58534: dsymutil support for DW_OP_convert

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

Few nits in addition to the stuff we discussed offline.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFAttribute.h:47
+  /// DWARF expression.
+  static bool mayHaveLocationDescription(dwarf::Attribute Attr);
+
----------------
nit/subjective: I prefer `mayHaveLocationDescription`


================
Comment at: llvm/tools/dsymutil/DwarfLinker.cpp:1066
+    auto Op1 = Description.Op[1];
+    using Encoding = DWARFExpression::Operation::Encoding;
+    if ((Op0 == Encoding::BaseTypeRef && Op1 != Encoding::SizeNA) ||
----------------
Let's do this at the beginning of the functions. 


================
Comment at: llvm/tools/dsymutil/DwarfLinker.cpp:1071
+
+    if ((Op0 == Encoding::BaseTypeRef && Op1 == Encoding::SizeNA) ||
+        (Op1 == Encoding::BaseTypeRef && Op0 == Encoding::Size1)) {
----------------
Can we invert this and do a continue?


================
Comment at: llvm/tools/dsymutil/DwarfLinker.h:203
                                uint64_t ModulesEndOffset, unsigned &UnitID,
-                               unsigned Indent = 0, bool Quiet = false);
+                               bool IsLittleEndian, unsigned Indent = 0,
+                               bool Quiet = false);
----------------
Can we use the `endianness` enum from Support/Endian.h?


================
Comment at: llvm/tools/dsymutil/DwarfLinker.h:368
+    /// Clone a DWARF expression that may be referencing another DIE.
+    void cloneExpression(DIEValueList *Attr, ArrayRef<uint8_t> Expr,
+                         const DebugMapObject &DMO, CompileUnit &Unit,
----------------
Can you include why these two are different in the doxygen comment?


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

https://reviews.llvm.org/D58534