Skip to content
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

Switch warnings no longer work #2572

Closed
kripken opened this issue Jan 6, 2020 · 1 comment · Fixed by #2577
Closed

Switch warnings no longer work #2572

kripken opened this issue Jan 6, 2020 · 1 comment · Fixed by #2577

Comments

@kripken
Copy link
Member

kripken commented Jan 6, 2020

See discussion in #2451 (comment)

IIUC the issue is that we cast those class instances to an int, and the warnings don't work for ints.

Maybe there's another way? Or, if we don't want these warnings anymore, perhaps we can remove unnecessary cases from switches?

@tlively
Copy link
Member

tlively commented Jan 7, 2020

This is an issue both before and after #2434, but that PR brings it closer to being fixed.

The compiler feature that warns about non-exhaustive switches on enums only works when the type being switched on is that enum. But since #2459, switching on types has worked because the following automatic conversion is defined for Type:

// Allows for using Types in switch statements
constexpr operator uint32_t() const { return id; }

So the type actually being switched on is uint32_t. I just did a little experiment locally and it looks like switching the conversion operator to return ValueType rather than uint32_t in conjunction with #2434 will solve this problem. The question is then what to do about multivalue types, which don't correspond to any ValueType. I suppose the most robust thing to do would be to assert that the value is not a multivalue in the conversion operator. Multivalue types can't participate in switches anyway because their IDs are not constexpr.

tlively added a commit to tlively/binaryen that referenced this issue Jan 7, 2020
Fixes WebAssembly#2572 by changing the conversion used when switching on types
from uint32_t to ValueType.
tlively added a commit to tlively/binaryen that referenced this issue Jan 8, 2020
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.
aheejin added a commit to aheejin/binaryen that referenced this issue 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.
tlively added a commit that referenced this issue Jan 9, 2020
* Remove implicit conversion operators from Type

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.

* getVT => getSingle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants