[PATCH] D27170: Abstract almost all DwarfDebug out of the classes in DIE.cpp.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: tools/dsymutil/DwarfLinker.cpp:218-219
 
-  CompileUnit(CompileUnit &&RHS)
-      : OrigUnit(RHS.OrigUnit), Info(std::move(RHS.Info)),
-        CUDie(std::move(RHS.CUDie)), StartOffset(RHS.StartOffset),
-        NextUnitOffset(RHS.NextUnitOffset), RangeAlloc(), Ranges(RangeAlloc) {
-    // The CompileUnit container has been 'reserve()'d with the right
-    // size. We cannot move the IntervalMap anyway.
-    llvm_unreachable("CompileUnits should not be moved.");
-  }
+  CompileUnit(const CompileUnit &RHS) = delete;
+  CompileUnit(CompileUnit &&RHS) = delete;
 
----------------
This type includes a non-copy/movable DIEUnit, so it's implicitly non-copy/movable, so you can omit these if you like ( http://blog.florianwolters.de/educational/2015/01/31/The_Rule_of_Zero/ ) I think


================
Comment at: tools/dsymutil/DwarfLinker.cpp:1099-1100
 
+typedef std::unique_ptr<CompileUnit> CompileUnitStorage;
+typedef std::vector<CompileUnitStorage> CompileUnitCollection;
 /// \brief The core of the Dwarf linking logic.
----------------
Think the code might be more legible without these typedefs (the first moreso than the second)


================
Comment at: tools/dsymutil/DwarfLinker.cpp:1100
+typedef std::unique_ptr<CompileUnit> CompileUnitStorage;
+typedef std::vector<CompileUnitStorage> CompileUnitCollection;
 /// \brief The core of the Dwarf linking logic.
----------------
Alternatively, this could be a std::deque if items are added but never removed & object identity is important. (may not be better that way - just suggesting it so you, Adrian, etc can consider it)


================
Comment at: tools/dsymutil/DwarfLinker.cpp:2722
+  if (Die) {
+    assert(Info.Clone == nullptr);
+  } else {
----------------
Please add a message to the assertion to explain what/why this should be true.

(I personally find a block with only an assert in it to be a little weird and might tend to phrase this as "assert(!Die || !Info.Clone); if (!Die) { ... } - but yeah, it's not exactly more legible that way :/)


================
Comment at: tools/dsymutil/DwarfLinker.cpp:2724-2726
+    Die = Info.Clone;
+    if (!Die)
+      Die = Info.Clone = DIE::get(DIEAlloc, dwarf::Tag(InputDIE.getTag()));
----------------
aprantl wrote:
> this is confusing to read. No idea how to make it better though...
Might this be simpler as:

  if (!Info.Clone)
    Info.Clone = ...;
  Die = Info.Clone;


================
Comment at: tools/dsymutil/DwarfLinker.cpp:3470
       if (!registerModuleReference(*CUDie, *CU, ModuleMap))
-        Units.emplace_back(*CU, UnitID++, !Options.NoODR, "");
+        Units.push_back(CompileUnitStorage(new CompileUnit(*CU, UnitID++, !Options.NoODR, "")));
     }
----------------
prefer llvm::make_unique


https://reviews.llvm.org/D27170