[PATCH] D28828: [DWARF] [ObjectYAML] Adding APIs for unittesting

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:620-653
+    StringRef *SectionData = MapSectionToMember(name);
     if (SectionData) {
       *SectionData = data;
       if (name == "debug_ranges") {
         // FIXME: Use the other dwo range section when we emit it.
         RangeDWOSection = data;
       }
----------------
dblaikie wrote:
> You can roll the variable declaration into the 'if' here, if you like
Did this regress when you committed it (if I'm reading the patch/changes/Phabricator correctly) - looks like the StringSwitch came back, rather than using the common utility function, MapSectionToMember.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1187
   // Verify the number of compile units is correct.
   uint32_t NumCUs = DwarfContext.getNumCompileUnits();
   EXPECT_EQ(NumCUs, 1u);
----------------
This should probably be an ASSERT - otherwise it'll crash/UB on the next line anyway.

Did you chat to Lang about the fact that this would produce a follow-on assert if it failed? (since the code doesn't handle the error, if there is one)

Perhaps something like HandleExpectedError would be suitable/necessary here.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1188
   uint32_t NumCUs = DwarfContext.getNumCompileUnits();
   EXPECT_EQ(NumCUs, 1u);
   DWARFCompileUnit *U = DwarfContext.getCompileUnitAtIndex(0);
----------------
Could write this as **Obj, if you like.


Repository:
  rL LLVM

https://reviews.llvm.org/D28828