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

[Chris Bieneman via Phabricator via llvm-commits]

beanz 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:
> 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.
I think the diff just shows up strange. There is only one copy of the StringSwitch in the code I see checked in.


================
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);
----------------
dblaikie wrote:
> 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.
Sorry, forgot to update this. Lang is OOO and not easily reachable. I'll chat with him when he gets back in the meantime I'll update to ASSERT and commit.


Repository:
  rL LLVM

https://reviews.llvm.org/D28828