[NFC] Enforce use of Type:: on type names
#2434
Conversation
|
I prefer this over #2433. Enforcing use of |
|
How does this related to #2433? Do we need just one of the two? |
src/wasm-type.h
Outdated
| // is different, as it can be "ignored" when doing type checking across | ||
| // branches | ||
| unreachable | ||
| class Type { |
There was a problem hiding this comment.
Wouldn't just switching to enum class get you most of the benefits?
|
Perhaps switching to enum class first, which would keep 99% of this change the same might work then? That was you can do a followup proposal to make it into an actual class it might be easier to review (since it would eliminate a lot of the noise from the this change)? Just a thought. |
|
I was hoping to get #2433 merged first, which is a small PR that should be easy to review. Then I would rebase this PR on top of that. |
|
Ah yes, that makes sense. Sorry for the noise. |
As the number of types and the length of their names grow, it becomes less and less desirable to have them in the global wasm namespace. This PR removes the global constexprs that mirror the value type enum elements inside the `Type` class to enforce the use of the `Type::` prefix everywhere the value types are used directly.
tlively
force-pushed
the
namespaced-types
branch
from
88f7f4f to
e83e246
Compare
|
I still have to go through this and reformat comments, but when that is done this will be ready for review. |
| constexpr Type nullref = Type::nullref; | ||
| constexpr Type exnref = Type::exnref; | ||
| constexpr Type unreachable = Type::unreachable; | ||
|
|
There was a problem hiding this comment.
this enforcees that we must use the Type:: notation, is that right?
There was a problem hiding this comment.
That's correct. I'll follow up separately on #2572; it isn't materially affected by this PR.
These seem to be accidentally introduced in when we enforced use of `Type::` on type names in WebAssembly#2434. By the way TIL this actually compiles: ``` Type::Type::Type::Type::Type::Type::Type::Type::none ```
These seem to be accidentally introduced in when we enforced use of `Type::` on type names in WebAssembly#2434. By the way TIL this actually compiles: ``` Type::Type::Type::Type::Type::Type::Type::Type::none ```
These seem to be accidentally introduced in when we enforced use of `Type::` on type names in WebAssembly#2434. By the way TIL this actually compiles, and don't know why: ``` Type::Type::Type::Type::Type::Type::Type::Type::none ```
These seem to be accidentally introduced in when we enforced use of `Type::` on type names in WebAssembly#2434. By the way TIL this actually compiles, and don't know why: ``` Type::Type::Type::Type::Type::Type::Type::Type::none ```
These seem to be accidentally introduced in when we enforced use of `Type::` on type names in #2434. By the way TIL this actually compiles, and don't know why: ``` Type::Type::Type::Type::Type::Type::Type::Type::none ```
There's some test failure in this change even though it is supposed to be nonfunctional. But before I fix that I thought I would see if this is even a change people are interested in making.
Please don't spend time going through this methodically right now. Just initial impressions would be good.