Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improved WasmPtr, added WasmCell #2442

Merged
merged 17 commits into from Jul 2, 2021
Merged

Improved WasmPtr, added WasmCell #2442

merged 17 commits into from Jul 2, 2021

Conversation

syrusakbary
Copy link
Member

@syrusakbary syrusakbary commented Jun 24, 2021 •
edited

Description

This PR improves a bit our WasmPtr API by making it a bit more safer and reliable.

Breaking changes

  • WasmPtr.deref will now return WasmCell<'a, T> instead of &'a Cell<T>
  • WasmPtr.deref_mut is no needed any longer

MarkMcCaskey
MarkMcCaskey approved these changes Jun 24, 2021
View reviewed changes
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is workable if we're good with the breakage in API and the performance decrease for the array method. I think this is a reasonable path forward: we can always add more APIs later for better performance.

lib/api/src/cell.rs Outdated Show resolved Hide resolved
lib/api/src/ptr.rs Outdated Show resolved Hide resolved
@syrusakbary
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Jun 24, 2021
@bors @syrusakbary
Merge #2442
e1c234d 
2442: Improved WasmPtr, added WasmCell r=syrusakbary a=syrusakbary


# Description

This PR improves a bit our WasmPtr API by making it a bit more safer and reliable.

## Breaking changes
* `WasmPtr`.`deref` will now return `WasmCell<'a, T>` instead of `&'a Cell<T>`
* `WasmPtr`.`deref_mut` is no needed any longer

Co-authored-by: Syrus Akbary <me@syrusakbary.com>
MarkMcCaskey
MarkMcCaskey reviewed Jun 24, 2021
View reviewed changes
lib/api/src/ptr.rs Outdated Show resolved Hide resolved
lib/api/src/ptr.rs Outdated Show resolved Hide resolved
lib/types/src/memory_view.rs Outdated Show resolved Hide resolved
lib/types/src/memory_view.rs Outdated Show resolved Hide resolved
@MarkMcCaskey
Copy link
Contributor

bors r-

@bors
Copy link
Contributor

bors bot commented Jun 24, 2021

Canceled.

MarkMcCaskey
MarkMcCaskey suggested changes Jun 24, 2021
View reviewed changes
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to fix the major bugs first

MarkMcCaskey
MarkMcCaskey reviewed Jun 25, 2021
View reviewed changes
lib/api/src/ptr.rs Outdated Show resolved Hide resolved
lib/types/src/memory_view.rs Outdated Show resolved Hide resolved
lib/types/src/memory_view.rs Outdated Show resolved Hide resolved
lib/types/src/memory_view.rs Outdated Show resolved Hide resolved
lib/wasi/src/syscalls/mod.rs Outdated Show resolved Hide resolved
lib/wasi/src/syscalls/mod.rs Outdated Show resolved Hide resolved
lib/wasi/src/syscalls/mod.rs Outdated Show resolved Hide resolved
lib/wasi/src/syscalls/mod.rs Outdated Show resolved Hide resolved
lib/wasi/src/syscalls/mod.rs Outdated Show resolved Hide resolved
lib/wasi/src/syscalls/mod.rs Outdated Show resolved Hide resolved
@syrusakbary
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Jul 2, 2021
@bors
Try #2442:
449734c
@bors
Copy link
Contributor

bors bot commented Jul 2, 2021

try

Build failed:

@syrusakbary syrusakbary mentioned this pull request Jul 2, 2021
Merged
1 task
@syrusakbary
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Jul 2, 2021
@bors
Try #2442:
ebf7d5a
@bors
Copy link
Contributor

bors bot commented Jul 2, 2021

try

Build failed:

Hywan
Hywan reviewed Jul 2, 2021
View reviewed changes
lib/types/src/memory_view.rs Outdated Show resolved Hide resolved
lib/types/src/memory_view.rs Outdated Show resolved Hide resolved
@syrusakbary
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Jul 2, 2021
@bors
Try #2442:
1627d4d
@bors
Copy link
Contributor

bors bot commented Jul 2, 2021

try

Build succeeded:

@syrusakbary
Copy link
Member Author

I assume everything is good to go now, given that all feedback is addressed and it was previously approved. Merging manually :)

@syrusakbary syrusakbary merged commit 3604deb into master Jul 2, 2021
@bors bors bot deleted the features-cell branch July 2, 2021 23:07
This was referenced Jul 15, 2021
Closed
Closed
@syrusakbary syrusakbary added this to the v2.1 milestone Nov 5, 2021
@Michael-F-Bryan Michael-F-Bryan mentioned this pull request Jan 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hywan Hywan Hywan left review comments

@MarkMcCaskey MarkMcCaskey MarkMcCaskey requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@syrusakbary @MarkMcCaskey @Hywan