replace old style union in initializing expression representation with std::variant

[dbezhetskov]

Hi, this is just small cleanup.
I've replaced union with std::variant to free Type type because now it should be always POD as a member of union.
Suddenly, after refactoring I revealed that FuncRef and NullRef cases and Type actually not used in our code so I don't move them into new version of InitExpr.

[sbc100]

I've heard that using any/optional/variant could be problematic for macOS < 10.13... but I'm not sure if wabt has uses who care about that.

[sbc100]

I wonder if that fact that FuncRef and NullRef are undefined actually just about us missing test cases for them. I guess it doesn't hurt to land this either way.

[takikawa]

Are there any remaining issues with landing this? It would be helpful for completing the implementation of typed funcrefs and also GC support, as it would allow including a Var or similar data in the type.

[takikawa]

Are there any remaining issues with landing this? It would be helpful for completing the implementation of typed funcrefs and also GC support, as it would allow including a Var or similar data in the type.

It looks like this actually has to be rebased to be mergeable now, since the relevant file has been updated recently. I can work on rebasing this

[takikawa]

I've rebased this now on this branch takikawa@9048277

@dbezhetskov could you update your PR branch with this commit and re-push? Thanks.

[dbezhetskov]

I've updated the branch, thanks @takikawa

[sbc100]

Why not continue to call this imm which seems more descriptive?

Also, maybe we can call all the possible variantes accordingly: ImmI32, ImmI64, ImmF64, ImmV128, ImmIndex.

[sbc100]

Also can we avoid all the .value accessors below if we use typedefs here instead of nested structs:

typedef ImmI32 uint32_t;

?

[dbezhetskov]

Thanks for the comments Sam!
I've returned imm name and renamed structs.

Actually I can't reduce .value because in that case variant will have equal types. For example, we represent i64 and f64 as uint64_t. In that case we can't use std::visit and std::get<Type> because variant's options aren't unique, so one possible solution is to use indices:

using ImmI32 = uint32_t;
using ImmI64 = uint64_t;
using ImmF32 = uint32_t;
using ImmF64 = uint64_t;
using ImmV128 = v128;
using ImmIndex = Index;

constexpr std::size_t ImmI32TypeIdx = 0;
constexpr std::size_t ImmI64TypeIdx = 1;
constexpr std::size_t ImmF32TypeIdx = 2;
constexpr std::size_t ImmF64TypeIdx = 3;
constexpr std::size_t ImmV128TypeIdx = 4;
constexpr std::size_t ImmIndexTypeIdx = 5;

For that case we should manually keep them in sync with the order of types in the variant.
Moreover, beautiful std::visit for this case can't be used and then should be replaced with a chain of if-then-else, so I think structs are ok, wdyt?

[sbc100]

I don't understand the "variant's options aren't unique" part? Aren't different typedefs considered unique types?

[dbezhetskov]

I mean when we have typedefs or usings like this:

using ImmI32 = uint32_t;
using ImmI64 = uint64_t;
using ImmF32 = uint32_t;
using ImmF64 = uint64_t;
using ImmV128 = v128;
using ImmIndex = Index;

then std::variant<ImmI32, ImmI64, ImmF32, ImmF64, ImmV128, ImmIndex> translates into
std::variant<uint32_t, uint64_t, uint32_t, uint64_t, v128, Index>.
So, we can't use std::get<ImmI32> because it translates into std::get<uint32_t> because using/typedef doesn't introduce new types, it is just a type alias. In the end, std::get<uint32_t> is ambiguous because it can refer to a place of ImmI32 or refer to a place of ImmF32.

The documentations for std::get says: The call is ill-formed if T is not a unique element of Types....

[sbc100]

In that case maybe we can reduce the variant to just ImmU32, ImmU64 and ImmV128?

[dbezhetskov]

Actually we can, but in that case PrintInitExpr(const InitExpr& expr) can't distinguish between F64 and I64 without any additional enum opcode support (we removed it in this PR).

The main argument here is that we replace the (union + enum opcode) scheme with a single std::variant because it is more convenient to add new types and compiler can check that some options in std::visit are uncovered.

I don't see any memory or speed degradation from this change, at least in theory, because std::variant will have the same size as the previous union based solution with enum class InitExprType. With variant we just don't need to manually maintain InitExprType.

[binji]

This seems like a smart change for the reasons @takikawa mentioned. I personally don't really like using std::visit and overloaded very much since it is a little complex, and would prefer using a switch/index instead. But it's mostly just a personal opinion, and I see that using overloaded does provide better type safety.

[sbc100]

This seems like a smart change for the reasons @takikawa mentioned. I personally don't really like using std::visit and overloaded very much since it is a little complex, and would prefer using a switch/index instead. But it's mostly just a personal opinion, and I see that using overloaded does provide better type safety.

I think I agree. I prefer the old switch style. Especially if that allows use to avoid adding all those new struct types.

[keithw]

In the interests of trying to clean up the PR backlog -- is there still interest in pursuing this (subject to comments) or is this ok to close?

[takikawa]

Hi @keithw, thanks for checking in. I personally don't have the bandwidth to pursue this currently and am ok with closing it. WDYT @dbezhetskov?

[dbezhetskov]

The same for me, feel free to close this.

[keithw]

Thanks for the quick responses! OK, closing for now.