[PATCH] D101341: Initialize optional members of ELFYAML types.

[Vyacheslav Zakharin via Phabricator via llvm-commits]

vzakhari added a comment.

In D101341#2781231 <https://reviews.llvm.org/D101341#2781231>, @jhenderson wrote:

> In D101341#2755605 <https://reviews.llvm.org/D101341#2755605>, @vzakhari wrote:
>
>> In D101341#2753240 <https://reviews.llvm.org/D101341#2753240>, @jhenderson wrote:
>>
>>> @vzakhari, what's the motivation for doing this? These data structures are used within yaml2obj/obj2yaml, although I'm not particularly familiar with the plumbing to identify whether it matters. My suspicion is that it doesn't.
>>
>> @jhenderson, there are two ways to create a YAML file using YAML mappings.  The first one is to create a YAML file from a string bufffer (see line 38 in `llvm/unittests/ObjectYAML/ELFYAMLTest.cpp`).  The second way is to populate the `ELFYAML::` data structures and stream them into `yaml::Output`.  Both ways are valid.  The second way is a little bit more convenient for programmatic ELF-YAML creation in my downstream application.  When I try to use `ELFYAML::` data structures, there is no convenient way to make sure that all optional members in them are initialized.  Neither default nor zero-initialization works.  This is why I made these changes to make sure that all optional members are initialized properly.  Note that the default values are not necessarily zeroes.
>
> Which cases don't have zero as their default value? The ones you've modified in this patch all are (even the Mips enums are - the enum values are value 0).

Sorry, I do not see where I said that there are no default values.  I said that neither default nor zero-initialization works - in this phrase I referred to initialization of POD members of objects, when we use default or zero-initialization.  For example, look at `struct MipsABIFlags`: if we declare an object of this type using default initialization/construction, then members like `ISARevision` will be default constructed, which in turn causes their value to be undefined (because they use the default constructor of the type defined by `LLVM_YAML_STRONG_TYPEDEF`).  Value/zero-initialization will also not work for such objects, because `struct MipsABIFlags` has a user-defined constructor.

> Frankly speaking, I don't like the style of the change: it looks somewhat ugly (`= 0` would be better than `{0}` in my opinion at least), but more importantly there's no guarantee that it will continue to work - more fields could be added at a later date, for example, without this initialisation, especially as it's not clear when it needs to be explicitly initialised (this change for example doesn't change the `Size` field of the `StackSizeEntry` struct). It sounds like there's no internal-to-LLVM usage of this approach, so it could easily be considered unnecessary and get dropped at a later date too.

I can try to change `{0}` to `= 0`, if this helps getting this merged.  Please let me know.
Yes, there is not guarantee that newly added optional fields are properly initialized.  There is no way to assert this, just as many other things developers do.  Why does it matter?
The added unit test verifies that the existing optional fields are properly initialized, so this should be enough to make sure that the current changes are no dropped accidentally.

Again, I am just following the specification <https://llvm.org/docs/YamlIO.html#default-values> of YamlIO, which says: "That native class must have a default constructor. Whatever value the default constructor initially sets for an optional field will be that field’s value."
I believe this implies that the default constructors of YamlIO objects must set their optional fields to some not undefined value.


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

https://reviews.llvm.org/D101341