Conversation
f67ef57 to
04ab34d
Compare
d5af3e5 to
f050e53
Compare
| // internal name. | ||
| std::vector<Literals> definedGlobals; | ||
| std::vector<std::unique_ptr<RuntimeTable>> definedTables; | ||
| std::vector<Tag> definedTags; |
There was a problem hiding this comment.
Why do we need std::unique_ptr for definedTables but not for definedGLobals or definedTags? Or could we get rid of the unique ptr in a separate change?
There was a problem hiding this comment.
definedTables are unique_ptrs because RuntimeTable is a virtual type. It also makes a little more sense for tables than for globals and tags since tables are more like 'control' while globals and tags are more like plain data (setting aside what we discussed about tags having an identity).
We could potentially remove the indirection by changing it to RealRuntimeTable but it isn't possible right now because we inject a fake RuntimeTable in ctor eval: link. Once we resolve the TODO mentioned there, we could change it. On that note: what's the reasoning for wanting to remove the unique_ptr here? Is it speed or something else?
There was a problem hiding this comment.
Makes sense, thanks. My motivation for asking is that I think it would be nice to be as uniform as possible with how we treat the different kinds of module fields and their corresponding runtime objects here. So I would have expected the vector of defined tags to look very much like the vector of defined tables, i.e. both or neither using unique_ptrs. (This is also why I would expect the tags to be represented by RuntimeTag objects containing pointers to the instantiated Tag definitions, just like RuntimeTable has a pointer to a Table.)
I would go as far as to say that RuntimeTag doesn't need to support an equality operator or do anything except wrap a Tag*. We can tell if two RuntimeTags are the same if they physically have the same address.
There was a problem hiding this comment.
So is you point that because these are all module fields / importable things that they should all either be unique_ptrs or not unique_ptrs? I feel that they have that in common, but what they don't have in common is whether they encode data or behavior. Literals are closer to data and tables/memories/functions are more of behavior. e.g. for tables we disable copy construction and move assignment, but it's fine to do this with a Literal (and we do do that here). Also it's a little strange (although allowed) to do copy assignment on the pointed-at value of a unique_ptr, which the former code link would do if we changed definedGlobals to hold unique_ptrs.
The rule I'm following is that value objects can be held directly, while behavior objects disable copying/assignment and are held with unique_ptrs. From that perspective it's fine if globals aren't held in a unique_ptr.
I didn't mention tags because they're in an in-between state in this PR. Here I'm using a 'data' object to represent them, but it would mostly be wrong to copy them since it would change its identity. In the next PR I plan to add a type for them that can't be copied, or maybe I'd represent them with unique_ptrs instead.
Part of #8180 and #8261. Fixes the semantics/spec test when the same tag is imported in different instances, in which case the tag should behave as a new identity, which was previously not the case (see the tags in the modified instance.wast in this PR).