[PATCH] D73086: [DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Sorry, I've been quite busy with having about 15-20 different active reviews at once, some of which are not trivial in complexity. It may take me a couple of working days to respond to things.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:314
+                                  IsDWO ? *DataDWO : *Data, IsMacro)) {
+    MacroPtr.reset();
+    return std::move(Err);
----------------
dblaikie wrote:
> Rather than nulling out this pointer - use a local unique_ptr and assign to MacroPtr at the end if no errors have occurred. (safer than trying to remember/make sure all the failure paths undo the work)
It looks to me like @dblaikie wanted the `MacroPtr = std::make_unique<DWARFDebugMacro>();` line to be replaced with `auto Ptr = std::make_unique<DWARFDebugMacro>();`  and only assign to `MacroPtr` at the end.

If there is an error, then `MacroPtr` will remain unchanged (i.e. unitialised). This is very similar to having a strong exception safety guarantee in concept, i.e. the state hasn't changed if there's an error.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:297-298
+DWARFContext::getDWARFDebugMacro(std::unique_ptr<DWARFDebugMacro> &MacroPtr,
+                                 bool IsLittleEndian, bool IsDWO,
+                                 bool IsMacro) {
+  if (MacroPtr)
----------------
It might make the code nicer if `IsDWO` and `IsMacro` are folded into one small enum e.g. `MacroSecType`. You can then `switch` on that value, and it also avoids multiple boolean arguments, which are easy to get wrong.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:489
+    else
+      Macinfo.get()->dump(OS);
   }
----------------
SouraVX wrote:
> dblaikie wrote:
> > This (& similar elsewhere) should be written as "Macinfo->dump(OS)", the ".get()" is redundant.
> > 
> > @jhenderson - did you want to roll the error handling/dumping into the common code too? (looks like the "if/handle/else/dump" could go in the common function (& rename the function to parseAndDump or something) too?
> Should we go ahead with this ?
Since this is all within the same library, we can put the recoverable error handler calls inside the function, and simplify the interface further.

@dblaikie's first comment about `get` though should also be addressed.


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

https://reviews.llvm.org/D73086