Repository navigation
[NFC] Rename Type::isNull to Type::isBottomRef - #9288
Conversation
Rename Type::isNullable and Type::isNonNullable to Type::isNullableRef and Type::isNonNullableRef to make it clear that they already check that the type is a reference, and remove redundant isRef() checks at call sites.
Rename Type::isNull to Type::isBottomRef to make it clear that it checks for any reference to a bottom heap type (including uninhabited non-nullable ones) and already checks that the type is a reference, and clean up redundant checks at call sites.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request renames the isNull() method on Type to isBottomRef() and updates its usages across the codebase to clarify that it checks for a bottom reference type. The review feedback suggests improving code readability and reducing redundancy in DeadArgumentElimination.cpp and heap-types.cpp by using the existing GCTypeUtils::isUninhabitable helper function instead of manual checks.
| bool wasReturnUninhabitable = | ||
| func->getResults().isNull() && func->getResults().isNonNullableRef(); | ||
| func->getResults().isBottomRef() && func->getResults().isNonNullableRef(); |
There was a problem hiding this comment.
We can simplify this check by using the existing helper function GCTypeUtils::isUninhabitable from ir/gc-type-utils.h (you may need to include ir/gc-type-utils.h if it is not already included). This improves readability and reduces redundancy.
| bool wasReturnUninhabitable = | |
| func->getResults().isNull() && func->getResults().isNonNullableRef(); | |
| func->getResults().isBottomRef() && func->getResults().isNonNullableRef(); | |
| bool wasReturnUninhabitable = | |
| GCTypeUtils::isUninhabitable(func->getResults()); |
| for (size_t i = 0; i < children.size(); ++i) { | ||
| auto child = children[i]; | ||
| if (child.isNonNullableRef() && child.getHeapType().isBottom()) { | ||
| if (child.isNonNullableRef() && child.isBottomRef()) { |
There was a problem hiding this comment.
We can simplify this check by using the helper function GCTypeUtils::isUninhabitable(child) here as well, which makes the code more expressive.
| if (child.isNonNullableRef() && child.isBottomRef()) { | |
| if (GCTypeUtils::isUninhabitable(child)) { |
|
...I kind of feel the same about this as the other case? "is null" obviously includes "is a reference" because that is what a null is. I think shorter names are generally better. They are easier to read, save time, and more pleasant. |
|
The important change here isn't the addition of |
# Conflicts: # src/ir/gc-type-utils.h # src/passes/DeadArgumentElimination.cpp # src/passes/OptimizeInstructions.cpp # src/tools/fuzzing/heap-types.cpp # src/wasm/literal.cpp
|
Well, again I don't quite see things the same (it is a null type, just non-nullable; yes that is weird) but I won't object strongly. |
Rename Type::isNull to Type::isBottomRef to make it clear that it checks for any reference to a bottom heap type (including uninhabited non-nullable ones) and already checks that the type is a reference, and clean up redundant checks at call sites.