[PATCH] D101341: Initialize optional members of ELFYAML types.
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 12 15:10:17 PDT 2021
MaskRay added inline comments.
================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:661
struct MipsABIFlags : Section {
- llvm::yaml::Hex16 Version;
+ llvm::yaml::Hex16 Version{0};
MIPS_ISA ISALevel;
----------------
jhenderson wrote:
> 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.
We can change `LLVM_YAML_STRONG_TYPEDEF` or add a similar macro for integeral types - initialized to 0.
Then we can avoid `{0}` here and there.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101341/new/
https://reviews.llvm.org/D101341
More information about the llvm-commits
mailing list