Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Eliminate one shared static this from thread.d#1837

Merged
dlang-bot merged 1 commit intodlang:masterfrom
andralex:nosharedthisthread
Aug 9, 2017
Merged

Eliminate one shared static this from thread.d#1837
dlang-bot merged 1 commit intodlang:masterfrom
andralex:nosharedthisthread

Conversation

@andralex
Copy link
Member

@andralex andralex commented Jun 12, 2017

The Windows implementation does not need any initialization.

@andralex andralex force-pushed the nosharedthisthread branch 6 times, most recently from 6bf4a5a to c31ff35 Compare June 12, 2017 21:59
@andralex
Copy link
Member Author

ping

@CyberShadow CyberShadow requested a review from MartinNowak June 30, 2017 14:56
@andralex
Copy link
Member Author

andralex commented Jul 4, 2017

ping

@CyberShadow
Copy link
Member

CC @nrTQgc @nemanja-boric-sociomantic

@andralex I think this PR would be easier to review if you described the changes in more detail (ideally in the commit message), as the diff is hard to make sense of even with ?w=1.


pcParms.pc_cid = PC_CLNULL;
if (priocntl(idtype_t.P_PID, P_MYID, PC_GETPARMS, &pcParms) == -1)
assert( 0, "Unable to get scheduling class" );
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit pick: these asserts could have the unified syntax with version(Posix) - no ifs

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking over code, I don't get the idea for refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, me neither anymore. All good, sorry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see it now: https://github.com/dlang/druntime/pull/1837/files#diff-8bb12ed976acf0a5132e877ec5a01ea8R1021

priocntl(...) != -1 || assert (0, "Unable to get scheduling class.");

I don't think it's important, though.


static int loadGlobal(string which)() //@nogc nothrow pure @safe
{
shared Priority cache;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be static shared Priority cache?

Copy link
Member Author

Choose a reason for hiding this comment

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

@nemanja-boric-sociomantic ah, that was the whole idea :). Thanks!

@nemanja-boric-sociomantic
Copy link
Contributor

Other than that LGTM.

@nemanja-boric-sociomantic
Copy link
Contributor

Just to check, this comment:

The NetBSD version was elaborately just returning zero it seems, so I simplified that

is not valid anymore, right? (I can't see any changes done for NetBSD, which your second comment seems to confirm).

@Burgos
Copy link
Contributor

Burgos commented Jul 17, 2017

Ping :-)

@andralex
Copy link
Member Author

andralex commented Aug 3, 2017

@nemanja-boric-sociomantic yah, that comment about NetBSD is not valid, sorry for the confusion.

@andralex andralex force-pushed the nosharedthisthread branch from c31ff35 to bce06dd Compare August 3, 2017 21:01
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @andralex!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@andralex
Copy link
Member Author

andralex commented Aug 3, 2017

@nemanja-boric-sociomantic done

@andralex
Copy link
Member Author

andralex commented Aug 3, 2017

@CyberShadow added comments

@Burgos
Copy link
Contributor

Burgos commented Aug 3, 2017

Looks good, thanks! Needs squash and it's good to go AFAICS.

@nemanja-boric-sociomantic
Copy link
Contributor

Just needs squash @andralex

@andralex andralex force-pushed the nosharedthisthread branch from bce06dd to 81320e9 Compare August 9, 2017 17:19
@dlang-bot dlang-bot merged commit f3282fd into dlang:master Aug 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants