[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#2320986 <https://reviews.llvm.org/D89039#2320986>, @jhenderson wrote:

> I'm not sure either are independent clean-ups, personally. It seems like doing it right the first time is the way forward here, because it avoids having to duplicate a load of code in new places - it provides an easy way to extend the functionality for the cases that didn't have it before.

To perform a good cleanup I'll probably need to change the existent messages reported for sections that are not touched by this patch.
E.g, I'd like to rework the following to:

1. Be consistent with "Section size must be greater than or equal to the content size" message reported for other sections.
2. Allow case when neither `Entries`, `Content`, not `Size` is specified.

  if (const auto *SS = dyn_cast<ELFYAML::StackSizesSection>(C.get())) {
    if (!SS->Entries && !SS->Content && !SS->Size)
      return ".stack_sizes: one of Content, Entries and Size must be specified";
  
    // We accept Content, Size or both together when there are no Entries.
    if (!SS->Entries)
      return {};
  
    if (SS->Size)
      return ".stack_sizes: Size and Entries cannot be used together";
    if (SS->Content)
      return ".stack_sizes: Content and Entries cannot be used together";
    return {};
  }

Such cleanup is indeed unrelated to the change perfomed by this patch.

You can say that we can leave the `StackSizesSection` alone and do it for other sections, but
the more important problem probably is that `validate()` returns `StringRef` currently:
`StringRef MappingTraits<std::unique_ptr<ELFYAML::Chunk>>::validate(`

I guess the right way would be to use `StringSaver` (or, perhaps, to use `std::string`), but this is out of scope of this patch again.
This is not a trivial cleanup, so I think it should be done after (if should be).

> If you really want to split it up, I'd do it first, and make use of it in the cases that don't have the functionality at all, and then migrate the other places over to using the new functions.

Before this patch many sections did not support `Content` and `Size` together. There is almost nothing to cleanup there.


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

https://reviews.llvm.org/D89039