[PATCH] D32073: [ProfileData] Support cross target binary reading for coverage tool

Xinliang David Li via llvm-commits llvm-commits at lists.llvm.org
Fri Apr 14 09:39:54 PDT 2017


On Fri, Apr 14, 2017 at 8:56 AM, Vedant Kumar via Phabricator <
reviews at reviews.llvm.org> wrote:

> vsk added a comment.
>
> In https://reviews.llvm.org/D32073#727016, @davidxl wrote:
>
> > The main client (the compiler) should not need to know the details so a
> higher level interface taking 'Module' should be better.
>
>
> I see your point, but taking a 'Module' doesn't seem to be nicer, because
> it forces us to expose more APIs which are very slightly different from
> each other. This leaks details because in order to pick the right API, you
> need to know the low level details (e.g whether or not segment prefixes are
> needed).
>
>
The only distinction between two sets of APIs is compiler vs other host
tools.


> >> Wdyt of removing all of the get*SectionName helpers, and just exposing:
> getInstrProfSectionName(InstrProfSectKind IPSK, ObjectFileKind OFK, bool
> AddSegmentPrefix). It will make client code more verbose but very clear /
> unambiguous.
> >
> > It is the host tool that need to use the lower level interface. For  the
> later use, there was never a need for segment prefix, so adding the
> flexibility is not necessary.
>
> We could set 'bool AddSegmentPrefix = true' by default. This still
> provides some extra flexibility, but the right option is picked by default,
> so there should be less confusion.
>

If you prefer strongly about unifying the APIs, we should probably do this
after this feature settles. We (including you ;) ) can do this as a
followup patch and we can go from there. For now, I think it is better to
reduce churns.


>
> If you'd prefer to extend the set of get*SectionName() functions, I just
> ask that the names of the "getXSectionName(bool)" and
> "getXSectionName(Module *)" functions be made different.
>

Ok, I will try to come up with a different name.

David


>
>
> https://reviews.llvm.org/D32073
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170414/a55d2372/attachment.html>


More information about the llvm-commits mailing list