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

[jthread] Initial port #12497

Merged
merged 6 commits into from
Aug 5, 2020
Merged

[jthread] Initial port #12497

merged 6 commits into from
Aug 5, 2020

Conversation

RT2Code
Copy link
Contributor

@RT2Code RT2Code commented Jul 20, 2020

@JackBoosY JackBoosY added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jul 21, 2020
NancyLi1013
NancyLi1013 reviewed Jul 21, 2020
View reviewed changes
Copy link
Contributor

@NancyLi1013 NancyLi1013 left a comment

Choose a reason for hiding this comment

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

Is this a header-only library?
It seems that the repo is an unofficial website. Could you please use an official one?

@RT2Code
Copy link
Contributor Author

RT2Code commented Jul 21, 2020 •
edited
Loading

Yes, this is a header only library.

I don't understand what you mean about the unofficial website. jthread is an upcoming feature from the new C++20 standard not currently available in Visual Studio, which is why I made this port. It was proposed by Nicolai Josuttis and I'm using the repository where he submitted his implementation of it.

From : https://medium.com/@vgasparyan1995/a-new-thread-in-c-20-jthread-ebd121ae8906

Even though it comes with C++20, std::jthread doesn’t use any of the new language features, so it’s practically available even now. Here is an implementation of it by Nicolai Josuttis, the person who proposed it. Two header files from this repository are all you need to fully access this new feature: “jthread.hpp” and “stop_token.hpp” in “sources/”.

NancyLi1013
NancyLi1013 suggested changes Jul 21, 2020
View reviewed changes
ports/jthread/CONTROL Outdated Show resolved Hide resolved
ports/jthread/portfile.cmake Outdated Show resolved Hide resolved
ports/jthread/portfile.cmake Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

@RT222 Thanks for your detailed information.
Sorry, I mistook this port as JThread https://github.com/j0r1/JThread. I thought they were the same library before.
You can ignore my last comment about official website. The URL you provided seems fine.

RT2Code and others added 3 commits July 21, 2020 20:26
@RT2Code @NancyLi1013
[jthread] Add header only comment
b90b578 
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
@RT2Code @NancyLi1013
[jthread] Fix version date
c9cab16 
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
@RT2Code @NancyLi1013
[jthread] Move headers to include directory
15efefa 
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jul 22, 2020
@strega-nil
Copy link
Contributor

Given that there are two libraries named jthread, this name is probably too generic; could you rename this josuttis-jthread?

@NancyLi1013 NancyLi1013 added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Jul 29, 2020
@RT2Code
Copy link
Contributor Author

RT2Code commented Jul 29, 2020 •
edited
Loading

Isn't jthread for this one, and j0r1-jthread for the other one that you mentioned better? As the former is the implementation of the upcoming C++ 20 feature of the same name, I think it is the most appropriate candidate to be simply named jthread.

@strega-nil
Copy link
Contributor

Since this library is no more "official" than any other jthread library, it's totally reasonable for anyone to release a jthread library and call it jthread. Since that is the case, it makes more sense to me to namespace it with the author's name.

@RT2Code
Copy link
Contributor Author

RT2Code commented Jul 29, 2020 •
edited
Loading

I made the change you asked, but I still disagree with the "no more official" part, given that we are talking about the reference implementation of a standard C++ feature on a C++ package manager. :(

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Aug 4, 2020
@strega-nil
Copy link
Contributor

@RT222 I get your opinion; thanks for listening anyways ❤️

@strega-nil strega-nil merged commit 3d9934a into microsoft:master Aug 5, 2020
@RT2Code RT2Code deleted the jthread branch August 8, 2020 00:44
hellozee pushed a commit to hellozee/vcpkg that referenced this pull request Sep 11, 2020
@RT2Code @NancyLi1013 @hellozee
[jthread] Initial port (microsoft#12497)
3808649 
* [jthread] Initial port

https://github.com/josuttis/jthread

* [jthread] Add header only comment

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>

* [jthread] Fix version date

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>

* [jthread] Move headers to include directory

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>

* [jthread] Rename the port josuttis-jthread

* [jthread] Fix control port name

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Co-authored-by: Rémy Tassoux <rt2@rasterizedworld.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NancyLi1013 NancyLi1013 NancyLi1013 approved these changes

Assignees

@NancyLi1013 NancyLi1013

Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@RT2Code @NancyLi1013 @strega-nil @JackBoosY