Remove ValueWithMetadata#1057
Conversation
👷 Deploy Preview for salsa-rs processing.
|
Veykril
left a comment
There was a problem hiding this comment.
Yea I think this is actually fine now. We used to exclusively borrow the user supplied value in some codepaths which would cause aliasing or the metadata fields requiring this weird split. But after rebasing the PR a couple times it seems those issues resolved themselves now, I at least do not see places where we run risk of aliasing by removing this split again now
|
I'd like to have @ibraheemdev double check that observation though just in case :) |
I wouldn't mind that as it at least makes clear why the split exists in the first place and gives us a chance to document what constraints must be upheld when reading or writing the field. Do we need the |
|
I'm not sure, I haven't looked at the invariants closely. I think it's fine to merge this for the memory usage improvements and leave a TODO for later, it's not a huge deal. |
This might be too naive, but it's not clear to me what the benefit of
ValueWithMetadatais.It seems we can inline
ValueintoValueWithMetadata.Closes #1053
Test plan
Verified that this reduces the struct memory usage (it's negligible over all but still)
master
This PR
When running ty on trio