[PATCH] D78736: [DWARF5]: Added support for dumping strx forms in llvm-dwarfdump

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX added a comment.

In D78736#2007450 <https://reviews.llvm.org/D78736#2007450>, @ikudrin wrote:

> In D78736#2005497 <https://reviews.llvm.org/D78736#2005497>, @SouraVX wrote:
>
> > > It starts looking that it is better to have two distinct methods in the public interface, parseMacinfo() and parseMacro(), than adding lots of the optional parameters. They can still share the implementation, though.
> >
> > Yes, I'm also noticing it. But this can be tricky(since we want code sharing too) these `macro` and `macinfo` is using 4 common forms. I'll be separately investigating this to cleanly refactor it so that the public interface should be more clean/intuitive. I think a private member function `parseImpl` to be called by public `parseMacinfo/Macro` would be the best fit?
> >
> > - This is just an abstract idea, have to dig-in more for implementation. May be 1/2 other patches ?
>
>
> We can go with a private `parseImpl()` for now. It may be improved later.


Thanks for sharing your thoughts @ikudrin!
Should we do this refactoring after these 4 patches are in and macro infrastructure is all set ? Or should we delay these patches and focus on refactoring first ? Advantage of the former approach would be that we'll have all ingredients(`macro macinfo split & non-split`) already and then doing the refactoring.
Could you please share your thoughts on this too! Thanks!


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

https://reviews.llvm.org/D78736