Skip to content

Add _thread.start_new_thread#1936

Merged
coolreader18 merged 11 commits into
masterfrom
coolreader18/_thread-start_new_thread
May 25, 2020
Merged

Add _thread.start_new_thread#1936
coolreader18 merged 11 commits into
masterfrom
coolreader18/_thread-start_new_thread

Conversation

@coolreader18

@coolreader18 coolreader18 commented May 17, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Dependent on #1933 and #1935

@coolreader18 coolreader18 force-pushed the coolreader18/_thread-start_new_thread branch 6 times, most recently from 5df519d to 94dccb3 Compare May 22, 2020 17:35
@coolreader18 coolreader18 marked this pull request as ready for review May 22, 2020 17:35
palaviv
palaviv approved these changes May 23, 2020
View reviewed changes

@palaviv palaviv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great. Can you add a small snippet to see this running?

@coolreader18

coolreader18 commented May 23, 2020

Copy link
Copy Markdown
Member Author

test_threading does some of that already, but I think I could probably verify the ordering now/make sure that it's actually concurrent somehow.

@coolreader18 coolreader18 force-pushed the coolreader18/_thread-start_new_thread branch from 94dccb3 to a6f6b1a Compare May 24, 2020 00:37
@coolreader18 coolreader18 force-pushed the coolreader18/_thread-start_new_thread branch from a6f6b1a to d12a793 Compare May 24, 2020 01:14
@coolreader18

coolreader18 commented May 24, 2020

Copy link
Copy Markdown
Member Author

@palaviv I was actually able to get test.test_thread passing with not too much effort, so I think that should work for tests. I guess the _thread module really doesn't have that much API surface.

palaviv
palaviv approved these changes May 24, 2020
View reviewed changes

@palaviv palaviv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great. I missed that there are already tests for this.

Comment thread vm/Cargo.toml Outdated
crossbeam-utils = "0.7"
generational-arena = "0.2"
parking_lot = { git = "https://github.com/Amanieu/parking_lot" }
parking_lot = { git = "https://github.com/coolreader18/parking_lot", branch="raw-re-is_locked" }

@palaviv palaviv May 24, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was a bit iffy when we added the parking_lot from Github. For now at least add a TODO to change this.

@coolreader18 coolreader18 May 25, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That was just temporary, the PR for that is merged now. I'll still add a TODO for the official git version, though, hopefully that will be published on crates.io soon.

@coolreader18 coolreader18 force-pushed the coolreader18/_thread-start_new_thread branch from d12a793 to 90223d8 Compare May 25, 2020 02:03
@coolreader18 coolreader18 merged commit e14adc4 into master May 25, 2020
@coolreader18 coolreader18 deleted the coolreader18/_thread-start_new_thread branch May 25, 2020 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@palaviv palaviv palaviv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@coolreader18 @palaviv