This patch LGTM. Don't spend hours of time working on supporting .pubnames/.pubtypes however, as they aren't widely used by my understanding. Still, for completeness, it doesn't hurt to support them for now.

This revision is now accepted and ready to land.Jun 22 2020, 7:25 AM

Harbormaster completed remote builds in B61219: Diff 272405.Jun 22 2020, 8:02 AM

In D82296#2106599, @jhenderson wrote:

This patch LGTM. Don't spend hours of time working on supporting .pubnames/.pubtypes however, as they aren't widely used by my understanding. Still, for completeness, it doesn't hurt to support them for now.

Yeah, I will not spend too much time on them. In the next patch, I will replace the InitialLength with Format and Length just as what we do in other sections. As for supporting multiple sets of entries, if nobody requests it, I will not work on it.

In D82296#2108025, @Higuoxing wrote:

In D82296#2106599, @jhenderson wrote:

This patch LGTM. Don't spend hours of time working on supporting .pubnames/.pubtypes however, as they aren't widely used by my understanding. Still, for completeness, it doesn't hurt to support them for now.

Yeah, I will not spend too much time on them. In the next patch, I will replace the InitialLength with Format and Length just as what we do in other sections. As for supporting multiple sets of entries, if nobody requests it, I will not work on it.

I think if you end up with some spare time towards the end of the project, it doesn't hurt to "finish this off". Just not at the cost of more useful features. I do agree with the Length change though. Consistency is quite important.

Closed by commit rG8c7775e9a734: [ObjectYAML][ELF] Add support for emitting the .debug_pubnames section. (authored by Higuoxing). · Explain WhyJun 23 2020, 5:49 AM

This revision was automatically updated to reflect the committed changes.

aprantl added inline comments.Jun 26 2020, 10:17 AM