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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:855
+                               /*IsMacro=*/true))
+    return std::move(Err);
+  return Macro.get();
----------------
SouraVX wrote:
> jhenderson wrote:
> > ikudrin wrote:
> > > If you call this method twice, and the first call returns an error, the second call will still return some partially parsed `DWARFDebugMacro` object. This is inconsistent. You should call `Macro.reset()` before returning an error.
> > > 
> > > The same for two other similar methods.
> > This sort of change is exactly why I think we should avoid duplicating the logic in three places. It would have been easy to add it only in one place.
> I'm investigating the opportunity you discussed/mentioned in last comment(Thanks for that!) If found with constructive cases, will push a patch or two for review.
> This sort of change is exactly why I think we should avoid duplicating the logic in three places. It would have been easy to add it only in one place.

Totally agree.


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

https://reviews.llvm.org/D73086