Stream: git-wasmtime

Topic: wasmtime / PR #6421 Add `EntityList::copy_from`

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2023 at 16:53):

PROMETHIA-27 opened PR #6421 from PROMETHIA-27:entity_list_copy_from to bytecodealliance:main:

This PR adds a new method to EntityList called copy_from, which works very similarly to copy_within on Vec in stdlib. It allows copying a slice from one EntityList to a given index in another, with only one reference to a ListPool. If None is passed for the other EntityList argument, it copies from within a single list.

This method allows efficiently copying between and within EntityLists without requiring unsafe (of which I'm not sure there are any sound public-facing approaches) or intermediate Vecs to store T in.

I'm not entirely confident that I've implemented this fully correctly, but I'm unaware of any ways in which it is incorrect and the moderately complex test I've added passes, as well as all others (which shouldn't be affected anyway).

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2023 at 16:53):

PROMETHIA-27 requested elliottt for a review on PR #6421.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2023 at 16:53):

PROMETHIA-27 requested wasmtime-compiler-reviewers for a review on PR #6421.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2023 at 23:40):

PROMETHIA-27 updated PR #6421.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2023 at 23:44):

elliottt created PR review comment:

Which of these asserts is expected to panic?

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2023 at 23:51):

PROMETHIA-27 created PR review comment:

Neither; it would panic in the copy_from. I just copied them from the other test since they were examples that are now prohibited.

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 03:23):

elliottt created PR review comment:

Would you mind commenting the call where you expect the exception to happen, just so that it's super clear for anyone debugging the test?

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 12:00):

PROMETHIA-27 updated PR #6421.

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2023 at 15:37):

elliottt submitted PR review:

This looks good to me with comments addressed, thank you!

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2023 at 15:37):

elliottt created PR review comment:

Thank you for this assertion, it definitely makes it easier to see that the use of copy_within is fine. Do you plan to follow-up with a PR to remove the need for the assert?

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2023 at 15:37):

elliottt created PR review comment:

Could you add a note to the doc comment that the function will panic if other is the same as self?

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2023 at 15:37):

elliottt submitted PR review:

This looks good to me with comments addressed, thank you!

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2023 at 11:18):

PROMETHIA-27 created PR review comment:

I'll try to get around to it, but it might be a minute

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2023 at 11:18):

PROMETHIA-27 updated PR #6421.

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2023 at 11:18):

PROMETHIA-27 created PR review comment:

Done!

view this post on Zulip Wasmtime GitHub notifications bot (May 31 2023 at 01:43):

elliottt merged PR #6421.

Last updated: Nov 22 2024 at 16:03 UTC