[PATCH] D42960: [ELF] Add .eh_frame pieces to map file

[Rui Ueyama via llvm-commits]

On Tue, Mar 6, 2018 at 10:31 AM Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:

> Rui Ueyama via Phabricator via llvm-commits
> <llvm-commits at lists.llvm.org> writes:
>
> > ruiu added inline comments.
> >
> >
> > ================
> > Comment at: ELF/InputSection.h:347
> >
> > -std::string toString(const elf::InputSectionBase *);
> > +std::string toString(const elf::InputSectionBase *, uint64_t SecOffset
> = 0);
> >  } // namespace lld
> > ----------------
> > I don't think this is a good idea. lld::toString(T) is an overloaded
> function dispatched by its parameter type. But you are now defining a
> different function which you only use in MapFile.cpp. I'd move this back to
> the original file.
>
> My objection to the original implementation was that it was looking at
> the output of toString, effectively knowing exactly what toString does
> and breaking the modularity.
>
> Would you be OK with having
>
> std::string lld::someName(const InputSectionBase *Sec, uint64_t Offset);
>
> and implementing std::string lld::toString(const InputSectionBase *Sec)
> as someName(Sec, 0)?
>

 I'm lukewarm on that. To me, toString(T) is a stringize function for an
object of type T, and passing an additional information other than the
object itself is a deviation from that principle, as the given additional
information is not a part of the given object. If we need that in too many
places, I might change my mind, but at least for this patch I don't want to
do that.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180306/db91872a/attachment.html>