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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:295-297
+static Expected<const DWARFDebugMacro *>
+getDWARFDebugMacro(const DWARFContext &Context,
+                   std::unique_ptr<DWARFDebugMacro> &MacroPtr,
----------------
Returning a pointer to the parameter seems unnecessarily confusing - just return Error instead of Expected - callers can use the MacroPtr they passed in instead of retrieving the same thing from the return value.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:301
+    return MacroPtr.get();
+  MacroPtr.reset(new DWARFDebugMacro());
+  DWARFDataExtractor *Data =
----------------
Prefer make_unique:
```
MacroPtr = std::make_unique<DWARFDebugMacro>();
```


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:302-308
+  DWARFDataExtractor *Data =
+      IsMacro
+          ? new DWARFDataExtractor(Context.getDWARFObj(),
+                                   Context.getDWARFObj().getMacroSection(),
+                                   IsLittleEndian, 0)
+          : new DWARFDataExtractor(Context.getDWARFObj().getMacinfoSection(),
+                                   IsLittleEndian, 0);
----------------
This probably shouldn't be dynamically allocated either - if DWARFDataExtractor is move constructible, which I guess it should be, then:
```
DWARFDataExtractor Data = IsMacro ? DWARFDataExtractor(... ) : DWARFDataExtractor(...);
```
If DWARFDataExtractor can't be move constructible then Optional<DWARFDataExtractor> could be used:
```
Optional<DWARFDataExtractor> Data;
if (IsMacro)
  Data.emplace(...);
else
  Data.emplace(...);
```


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:310-311
+  // FIXME: Add support for debug_macro.dwo section.
+  DWARFDataExtractor *DataDWO = new DWARFDataExtractor(
+      Context.getDWARFObj().getMacinfoDWOSection(), IsLittleEndian, 0);
+  if (Error Err = MacroPtr->parse(Context.getStringExtractor(),
----------------
This doesn't look like it needs to be dynamically allocated - could it just be:
```
DWARFDataExtractor DataDWO(Context.getDWARFObj().getMacinfoDWOSection(), IsLittleEndian, 0);
```


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:314
+                                  IsDWO ? *DataDWO : *Data, IsMacro)) {
+    MacroPtr.reset();
+    return std::move(Err);
----------------
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)


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

https://reviews.llvm.org/D73086