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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked an inline comment as done.
SouraVX added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:855
+                               /*IsMacro=*/true))
+    return std::move(Err);
+  return Macro.get();
----------------
ikudrin wrote:
> 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.
I already had discussion with @jhenderson WRT to this in previous revision, highlighted some of the implications https://reviews.llvm.org/D73086#1921493
+ I'm separately investigating that.


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

https://reviews.llvm.org/D73086