v.ast: add Table.update_sym_by_idx/2#25373
Conversation
spytheman
left a comment
There was a problem hiding this comment.
The new method does not use the whole map, it just needs its length.
|
Can you please also add a test case? |
| // delete_cached_type_to_str remove a `type_to_str` from the cache | ||
| pub fn (t &Table) delete_cached_type_to_str(typ Type, import_aliases_len int) { | ||
| cache_key := (u64(import_aliases_len) << 32) | u64(typ) | ||
| mut mt := unsafe { &Table(t) } |
There was a problem hiding this comment.
Why not just mut t Table for the receiver?
It is only called by update_sym_by_idx, which already has a mutable receiver 🤔 .
There was a problem hiding this comment.
I think this new method, will need to loop over all cached_type_to_str entries and delete the ones that have typ in the lower bits of the key.
Otherwise the strings for the same type, could be cached in different entries, only differing by their high parts of the key (which is depending on the declared aliases in each .v file).
Previously that was fine, since the type symbols were immutable, but if we allow updates, that cache can go stale, and lead to bugs, and we need to be careful to update or invalidate it properly.
There was a problem hiding this comment.
I.e. the new signature should be something like this:
pub fn (mut t Table) delete_cached_type_to_str(typ Type) {|
Thinking about it more, would not it be easier, to just register a placeholder symbol, while parsing the Then we would not have to invalidate any caches, and cgen could just skip the placeholder symbols completely. |
Think about it, dealing with array declarations like |
You may be right, but I do not understand why? In one case, you will have to update an existing symbol, and deal with the cache invalidations/updates (because currently the assumption is that the type/symbol information does not change much once registered). In the other, you will be copying an existing placeholder symbol on each recursion level, changing it in the process, and storing a new version of it (under a new type id), leaving the old one as it is - the cached entries in the table will continue to work for the placeholder types/symbols, and after the parsing is done, you would have a valid and complete type for the full array init. The negative could be using more memory for the new symbols, and potentially some logic in cgen to skip the incomplete ones. |
|
Anyway, given that you have a clear picture of how to do it with your approach, while I have not considered all implications of mine, I do not want to block it - I will merge this PR as it is, and we will see how it will pan out. If there are too many bugs found due to the cache maintenance, we could then reconsider etc. |
Allow update existing symbol via call
update_sym().a use case:
To support the new array syntax, we need to update the fixed-arrays size after scan the array lit. For example,
When parse this, we don't known the size of the fixed-array first, but register a
[-1][-1]intsymbol in the system.And then after scan the array lit,
[[1 2][3 4]], we can update the existing[-1][-1]intsymbol to[2][2]int.