[PATCH] D89039: [yaml2obj/obj2yaml] - Add support of 'Size' and 'Content' keys for all sections.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In D89039#2320874 <https://reviews.llvm.org/D89039#2320874>, @jhenderson wrote:

> Given this change, there's a lot of duplicated code across the various section varieties. There are two common patterns I see:

Yeah, I've sure noticed both of them too. Wanted to do something in a follow-up(s) with it.

> 1. Writing using the Content/Size and setting the sh_size field accordingly.
> 2. Validating that the Content/Size usage doesn't conflict with special entry lists (e.g. explicit relocations/notes etc).
>
> I think we can do better on both counts. For the first, I'd have a generic write method that checks to see if the base Section has a Size/Content field set, and if so, use that, return true if it did, false if not, so that the section-specific code knows whether to continue doing its thing.

There is a problem here. Our section writing code has the following structure often:

1. Set fields.
2. Handle Content/Size if any.
3. Handle Entries or other things if any.

E.g:

  SHeader.sh_info = Section.Info;
  
  if (Section.Content || Section.Size) {
    SHeader.sh_size = writeContent(CBA, Section.Content, Section.Size);
    return;
  }
  
  if (!Section.VerneedV)
    return;
  
  uint64_t AuxCnt = 0;
  for (size_t I = 0; I < Section.VerneedV->size(); ++I) {

We can replace the (2) with something, e.g:

    SHeader.sh_info = Section.Info;
  
    if (handleSizeAndContent(Section, SHeader)
      return;
  ...

But it still will be a piece of code in the middle of each `writeSectionContent` method I think. I'd do it separatelly if you think it worth doing (it hides just a bit of logic).

> For the second point, maybe we could have a templated method that takes the Entries as a templated parameter, as well as the other fields, and does the validation that way. It probably would need to take a string that indicates the name of the field.

Yes, I thought about something like this too. Except that I think it is possible to have a non-template lambda probably. Again, It looks like an independent follow-up cleanup?


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

https://reviews.llvm.org/D89039