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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

@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.



================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:661
 struct MipsABIFlags : Section {
-  llvm::yaml::Hex16 Version;
+  llvm::yaml::Hex16 Version{0};
   MIPS_ISA ISALevel;
----------------
vzakhari wrote:
> vzakhari wrote:
> > jhenderson wrote:
> > > vzakhari wrote:
> > > > MaskRay wrote:
> > > > > vzakhari wrote:
> > > > > > vzakhari wrote:
> > > > > > > MaskRay wrote:
> > > > > > > > Let strong typedef Hex16/MIPS_AFL_REG initialize the base value
> > > > > > > Sorry, I do not understand this.
> > > > > > > 
> > > > > > > Are you suggesting to let Hex16's default constructor to initialize the actual value?  I believe there is no default constructor that would do that.
> > > > > > > 
> > > > > > > 
> > > > > > > ```
> > > > > > >  #define LLVM_YAML_STRONG_TYPEDEF(_base, _type)                                 \
> > > > > > >      struct _type {                                                             \
> > > > > > >          _type() = default;                                                     \
> > > > > > >          _type(const _base v) : value(v) {}                                     \
> > > > > > >          _type(const _type &v) = default;                                       \
> > > > > > >          _type &operator=(const _type &rhs) = default;                          \
> > > > > > >          _type &operator=(const _base &rhs) { value = rhs; return *this; }      \
> > > > > > >          operator const _base & () const { return value; }                      \
> > > > > > >          bool operator==(const _type &rhs) const { return value == rhs.value; } \
> > > > > > >          bool operator==(const _base &rhs) const { return value == rhs; }       \
> > > > > > >          bool operator<(const _type &rhs) const { return value < rhs.value; }   \
> > > > > > >          _base value;                                                           \
> > > > > > >          using BaseType = _base;                                                \
> > > > > > >      };
> > > > > > > ```
> > > > > > > 
> > > > > > > Default construction will leave `value` uninitialized for POD `_base` type.  Zero-initializaton will set `value` to 0.
> > > > > > Just to clarify, I preferred value-initialization over zero-initialization to make all the initializations consistent, and since not all default values may be 0.
> > > > > You can initialize `_base value; `, then you don't need to spread `{0}` everywhere.
> > > > Can you please suggest the exact change?  Do you mean something like `_base value = _base()`?
> > > > 
> > > > In general, `_base` type may have a deleted default constructor, so this may not work.
> > > Are there any actual cases in current practice where the default constructor has been deleted?
> > I tried adding the in-class initialization as `_base value = _base()` and hit the following issue:
> > ```
> > llvm/include/llvm/ObjectYAML/WasmYAML.h:86:8: note: ‘llvm::WasmYAML::Import::Import()’ is implicitly deleted because the default definition would be ill-formed:
> >  struct Import {
> >         ^~~~~~
> > llvm/include/llvm/ObjectYAML/WasmYAML.h:92:12: error: union member ‘llvm::WasmYAML::Import::<unnamed union>::GlobalImport’ with non-trivial ‘llvm::WasmYAML::Global::Global()’
> >      Global GlobalImport;
> >             ^~~~~~~~~~~~
> > llvm/include/llvm/ObjectYAML/WasmYAML.h:93:11: error: union member ‘llvm::WasmYAML::Import::<unnamed union>::TableImport’ with non-trivial ‘llvm::WasmYAML::Table::Table()’
> >      Table TableImport;
> >            ^~~~~~~~~~~
> > llvm/include/llvm/ObjectYAML/WasmYAML.h:94:12: error: union member ‘llvm::WasmYAML::Import::<unnamed union>::Memory’ with non-trivial ‘constexpr llvm::WasmYAML::Limits::Limits()’
> >      Limits Memory;
> >             ^~~~~~
> > ```
> I believe this is a reproducer:
> ```
> struct A {
>   int x = int();
> };
> 
> struct B {
>   union {
>     A a;
>   };
> };
> 
> void foo() { B b = B(); }
> ```
> 
> ```
> ctr.cpp:11:20: error: call to implicitly-deleted default constructor of 'B'
> void foo() { B b = B(); }
>                    ^
> ctr.cpp:7:7: note: default constructor of 'B' is implicitly deleted because variant field 'a' has a non-trivial default constructor
>     A a;
>       ^
> 1 error generated.
> ```
> 
> `A` corresponds to the strongdef'ed type.
@MaskRay, do you have any suggestions on how to improve this. It looks to me like you had something in mind, but @vzakhari has been unable to identify it.


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

https://reviews.llvm.org/D101341