Skip to content

Comments

Rename: closure*table -> continuation*table#2690

Merged
mergify[bot] merged 7 commits intomasterfrom
gabor/rename
Jul 30, 2021
Merged

Rename: closure*table -> continuation*table#2690
mergify[bot] merged 7 commits intomasterfrom
gabor/rename

Conversation

@ggreif
Copy link
Contributor

@ggreif ggreif commented Jul 29, 2021

This is a wholesale rename, in the spirit of #2663 (comment)

@dfinity-ci
Copy link

This PR does not affect the produced WebAssembly code.

@ggreif ggreif requested review from crusso and osa1 and removed request for osa1 July 29, 2021 18:09
// Similar to `mark_root_mutbox_fields`, `closure_table_ptr_loc` is in static heap so it
// will be readable when we unthread closure table
thread(closure_table_ptr_loc);
if (*continuation_table_ptr_loc).unskew() >= heap_base as usize {
Copy link
Contributor

Choose a reason for hiding this comment

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

@osa1 Just to double check, your gc of the closure/continuation table doesn't assume anything about the Motoko object stored there right? It's just that they used to be scalars, closures or 2-element tuples and now may be scalars or 2 or 3 element tuples after @ggreif's recent change.

Copy link
Contributor

Choose a reason for hiding this comment

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

@crusso From gc point of view closure table is just a heap object that is also a root. We don't make any assumptions on the type of it, or how many elements it has, or whether elements are scalar or pointers.

@@ -7264,11 +7264,11 @@ and compile_exp (env : E.t) ae exp =

| OtherPrim "rts_callback_table_count", [] ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Ha! Maybe we should have called it the callback table! But let's stick with this for now.

Copy link
Contributor

@crusso crusso Jul 30, 2021

Choose a reason for hiding this comment

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

Do we want to rename the external Prims too, or just leave them for now? I'm actually ok with either since people aren't supposed to rely on these names... will leave it up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave them, this is an external interface.

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

But see comments.

@ggreif ggreif added DO-NOT-MERGE automerge-squash When ready, merge (using squash) and removed DO-NOT-MERGE labels Jul 30, 2021
@mergify mergify bot merged commit 6dfd0ae into master Jul 30, 2021
@mergify mergify bot deleted the gabor/rename branch July 30, 2021 10:28
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants