Skip to content

Remove implicit conversion operators from Type #2577

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 9, 2020

Conversation

tlively
Copy link
Member

@tlively tlively commented Jan 8, 2020

Now types must be explicitly converted to uint32_t with Type::getID or
to ValueType with Type::getVT. This fixes #2572 for switches that use
Type::getVT.

Now types must be explicitly converted to uint32_t with Type::getID or
to ValueType with Type::getVT. This fixes WebAssembly#2572 for switches that use
Type::getVT.
@tlively
Copy link
Member Author

tlively commented Jan 8, 2020

This is a more verbose but possible better alternative to #2575.

@tlively tlively requested review from aheejin and kripken and removed request for aheejin January 8, 2020 00:31
aheejin added a commit to aheejin/binaryen that referenced this pull request Jan 8, 2020
This adds `TYPE_SWITCH` macro that makes sure the given type is
converted to `enum Type::ValueType`. Using this over the regular
switch-case when switch-casing over types has an advantage that when new
types are added and not handled, they will generate compile time errors.

This is another attempt at solving WebAssembly#2572. We have WebAssembly#2577, and while I'm
OK with that, and this is just to see what other people might think.

This PR is at this stage to see what people think, and I only converted
a handful of switch-cases into `TYPE_SWITCH` for now for demonstration.
@aheejin
Copy link
Member

aheejin commented Jan 8, 2020

Hmm, I liked the interchangeable use cases between Type and Type::ValueType, and losing it is rather sad. I was wondering a macro in place of regular switch-case would work, and what I had in mind was something like #2579. I'm not completely satisfied with that either, given that we can't use the regular switch-case and can't enforce people to use the macro. Hmm. Maybe making it enforceable as in this PR might be safer?

@aheejin
Copy link
Member

aheejin commented Jan 8, 2020

By the way, one question: while we have to use .getVT() with switch-cases, can we still do something like if (type == Type::i32) when the type of type is Type? (This sentence reads funny, by the way)

constexpr ValueType getVT() const {
assert(!isMulti() && "Unexpected multivalue type");
return static_cast<ValueType>(id);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think getVT is a little unclear. For consistency with isSingle(), how about getSingle()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do 👍

@tlively
Copy link
Member Author

tlively commented Jan 8, 2020

By the way, one question: while we have to use .getVT() with switch-cases, can we still do something like if (type == Type::i32) when the type of type is Type? (This sentence reads funny, by the way)

Yes, this still works perfectly well because the ValueType on the right gets automatically promoted to Type.

@aheejin
Copy link
Member

aheejin commented Jan 8, 2020

Oh I see, so we need the conversion only when we need it as an enum or an unsigned int value.

Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with getSingle

@tlively
Copy link
Member Author

tlively commented Jan 9, 2020

Hmm, I just realized that getSingle won’t be a good name in a future with parameterized or imported types, since those types will represent single values but won’t have corresponding ValueTypes and won’t be able to be cases in switched over types. I’ll merge this for now and we can figure out a future-proof terminology separately.

@tlively tlively merged commit 7732943 into WebAssembly:master Jan 9, 2020
@tlively tlively deleted the exhaustive-switches-2 branch April 24, 2020 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch warnings no longer work
3 participants