Move vmspace into common plan #79
Conversation
steveblackburn
force-pushed
the
pr-movevmspace
branch
11 times, most recently
from
aaac380
to
7c55011
Compare
There was a problem hiding this comment.
This PR looks good and makes it quite clear what is specific to a plan and what is common. But we need to include builds and checks for the newly added vmspace feature, and fix any new issues raised by those checks. I will look at this PR again when the issues are fixed, just in case there is any major change.
| @@ -30,6 +30,7 @@ nogc = ["immortalspace", "largeobjectspace"] | |||
| semispace = ["immortalspace", "largeobjectspace", "copyspace"] | |||
|
|
|||
| # spaces | |||
| vmspace = [] | |||
There was a problem hiding this comment.
We need to include builds and style checks for the vmspace feature.
Can you add the following lines to our CI script?
.github/scripts/ci-build.sh:
cargo build --features nogc,vmspace
cargo build --features semispace,vmspace
.github/scripts/ci-style.sh:
cargo clippy --features nogc,vmspace
cargo clippy --features semispace,vmspace
And very possibly, you would need to fix the issues raised from those checks.
There was a problem hiding this comment.
No wait.... My mistake. I did not properly push my changes. Trying again now. Sorry for the false alarm. Still getting used to this workflow.
There was a problem hiding this comment.
Some part of the UI is confused because there were forced pushes and it cannot find the old commits anymore
There was a problem hiding this comment.
Ah OK. Then there's no need to do forced push. When you merge, you can use squash and merge, which results in only a single commit to master
There was a problem hiding this comment.
When you use forced push, it's harder to see the changes. For example, when you sent a link of one of the files for the unused_mut problem yesterday, I clicked the link and couldn't find the line you were referring to because the link pointed to a newer version of the file.
There was a problem hiding this comment.
I believe squash merge results in a singled commit that is authored by the person who pressed the merge button. Only in the comment it says it is co-authored the actual person who wrote the code. Otherwise I suggest we always use squash merge. We should test if this is true.
There was a problem hiding this comment.
OK. For my next PR, we can try this. I'll use multiple commits, and Yi can merge it. Should be later today.
There was a problem hiding this comment.
I just did some search and we might need to think about what workflow works better for us.
See isaacs/github#997 for example.
steveblackburn
force-pushed
the
pr-movevmspace
branch
from
7c55011
to
f867da0
Compare
steveblackburn
force-pushed
the
pr-movevmspace
branch
3 times, most recently
from
30962b7
to
be5fecd
Compare
caizixian
added
PR-ready-to-merge
PR-approved
steveblackburn
force-pushed
the
pr-movevmspace
branch
from
be5fecd
to
9e43942
Compare
This is the first step in a process of moving common functions out of the concrete plans.