Skip to content

Prefix Lift and Lower trait methods with memory_ #11070

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Jun 18, 2025

This is to distinguish them from the GC versions that will be added in follow up commits.

No functional changes here, just renaming.

@fitzgen fitzgen requested a review from a team as a code owner June 18, 2025 21:17
@fitzgen fitzgen requested review from alexcrichton and removed request for a team June 18, 2025 21:17
This is to distinguish them from the GC versions that will be added in follow up
commits.

No functional changes here, just renaming.
@fitzgen fitzgen force-pushed the add-memory-prefix-to-lift-lower branch from bda12c6 to 20927ed Compare June 18, 2025 21:21
@alexcrichton
Copy link
Member

To motivate a bit of a bikeshed: these sorts of changes tend to have a very high sticking power where they stick around for a very long time so I'd ideally like to handle things in one fell swoop instead of going through multiple renames. To bikeshed: memory_lift and memory_load are IMO deceptively subtle in how they're different as "lift from memory" is something I would reasonably say should be synonymous with a load from memory. Given that I'd prefer to find different names that are more nuanced than just putting memory_* in front of what we currently have.

One observation as well is that I think there's probably not going to be a gc_load or a gc_store. Current design has lifting/lowering of GC values being mostly like the "flat" lifting/lowering of linear memory values. I realize the struct/array cases are a bit different but I think what we'll probably do is pass a parameter through which differentiates the local/struct/array source/destinations of values.

Naming-wise WDYT about {lift,lower}_{linear,gc} and otherwise leaving load and store as-is? (or maybe {load,store}_linear)

@fitzgen
Copy link
Member Author

fitzgen commented Jun 18, 2025

For the GC methods I currently have:

  • Lift::gc_lift_val for doing GC lifting from a value
  • Lift::gc_lift_storage for doing GC lifting from a storage type (i.e. a struct field or array element)
  • Lower::gc_lower_val for doing GC lowering into a value
  • Lower::gc_lower_storage for doing GC lowering into a storage type

@fitzgen
Copy link
Member Author

fitzgen commented Jun 18, 2025

How about

  • Lift::lift_from_flat_linear
  • Lift::lift_from_memory_linear
  • Lift::lift_from_val_gc
  • Lift::lift_from_storage_gc
  • Lower::lower_to_flat_linear
  • Lower::lower_to_memory_linear
  • Lower::lower_to_val_gc
  • Lower::lower_to_storage_gc

?

@alexcrichton
Copy link
Member

That sounds good to me 👍

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants