[PATCH] D27634: Make a DWARFDIE class that can help avoid using the wrong DWARFUnit when extracting attributes

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h:29
 
-/// DWARFDebugInfoEntryMinimal - A DIE with only the minimum required data.
-class DWARFDebugInfoEntryMinimal {
+/// DWARFDebugInfoEntry - A DIE with only the minimum required data.
+class DWARFDebugInfoEntry {
----------------
Is this comment still accurate?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:61
+      return Die->getOffset();
+    return -1U;
+  }
----------------
Optional<uint32_t> ?
Optional<DwarfOffset>, to allow 64-bit? 


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:73
+    if (Die)
+      return Die->hasChildren();
+    return false;
----------------
Is it reasonable to call this if Die is null, or should this be an assertion?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:77
+  
+  bool isNULL() const {
+    return getAbbreviationDeclarationPtr() == nullptr;
----------------
operator bool() const?
Oh, or does this mean the NULL DIE that is at the end of each group?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:91
+  /// invalid DWARFDie instance if it doesn't.
+  DWARFDie getSibling() const {
+    if (isValid())
----------------
Perhaps Optional instead of default-construction?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:107
+  
+  /// Dump the DIE and all of its attribytes to the supplied stream.
+  ///
----------------
attributes


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:125
+  bool getAttributeValue(dwarf::Attribute Attr,
+                         DWARFFormValue &FormValue) const;
+  
----------------
Just a general remark: I wonder if these interfaces wouldn't be better if they returned Optionals instead of taking fail values, but I haven't looked at the call sites yet.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:295
+  /// of current DIE.
+  SmallVector<DWARFDie, 4>
+  getInlinedChainForAddress(const uint64_t Address) const;
----------------
Typically we write these functions as taking a `SmallVectorImpl&`, but I wonder if this isn't an artifact from before we could use `auto` in the code base.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:160
+      auto CUDIE = CU->getUnitDIE();
+      if (!CUDIE)
         continue;
----------------
these changes could be separate NFC commits. No need to pre-commit-review them.


https://reviews.llvm.org/D27634