-
Notifications
You must be signed in to change notification settings - Fork 835
Fix import checking when table's runtime size changes #8222
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
Conversation
d465928 to
f30f8c6
Compare
Part of #8180. We currently replicate the existing behavior. In the future, we can also change ctor-eval to really simulate tables, and only use the EvallingRuntimeTable in the case of table imports. Removes existing pointer chasing logic that we repeat for each table operation (e.g. tableLoad, tableStore). Allows us to fix spec tests where a table import becomes valid by resizing it in #8222.
0e6a253 to
cad1017
Compare
cad1017 to
1156ba6
Compare
| size() >= other.initial && tableMeta_.max <= other.max; | ||
| } | ||
|
|
||
| const Table* tableMeta() const { return &tableMeta_; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "meta" mean here? Could this be getTable() perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant it like 'metadata', I didn't want to name this table since this class is also a table. I think the Table struct represents something more like a table definition than a table itself, maybe I can rename this to tableDefinition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, yeah, maybe getDefinition() then? This is indeed the definition of the runtime table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I put it in the next PR if that sounds good #8230
Suggested in [#8222#discussion_r2721815229](#8222 (comment))
TableUtils::IsSubType is removed because it never makes sense to check for subtyping between two (static)
Tabledefinitions, since the runtime size must be taken into account.Fixes the
table_grow.wastspec test.Also make tableMeta() non-virtual in RuntimeTable since the base class has the field and it never makes sense to override it.