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.

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

Fix Issue 17231 and Improve std.concurrency.locate #5205

Closed
wants to merge 1 commit into from

Conversation

JackStouffer
Copy link
Member

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
17231 Function to get name from Tid

if (auto name = tid in namesByTid)
return *name;
return null;
}
Copy link
Contributor

@John-Colvin John-Colvin Mar 6, 2017

Choose a reason for hiding this comment

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

suggestion: replace the whole implementation with

synchronized (registryLock)
    return namesByTid.get(tid, null);

@@ -1076,6 +1076,24 @@ Tid locate( string name )
}
}

/**
* Gets all of the names associated with the $(LREF Tid).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest: Gets an array of all

* tid = The $(LREF Tid) to locate within the registry.
*
* Returns:
* The associated name or `null` if `tid` is not registered.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest: An array of the associated names

@John-Colvin
Copy link
Contributor

Would be nice to have a See Also section for the inverse and registering functions

@John-Colvin
Copy link
Contributor

Also, unittests

@JackStouffer
Copy link
Member Author

@John-Colvin Fixed

{
auto id = thisTid();
assert(register("Main", id));
assert(locate(id) == ["Main"]);
Copy link
Contributor

@John-Colvin John-Colvin Mar 7, 2017

Choose a reason for hiding this comment

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

This will fail if another unittest (or static this) has registered anything for the main thread. If you think that matters, you could wrap the whole unittest in spawn so you know you'll have a new id.

@JackStouffer
Copy link
Member Author

@John-Colvin Fixed again. Also added example for the other overload

@JackStouffer JackStouffer force-pushed the issue17231 branch 2 times, most recently from c15445d to c9260ac Compare March 7, 2017 19:14
@John-Colvin
Copy link
Contributor

Lgtm. If you want to do a real belt and braces job, check for what happens when there aren't any names registered, if an incorrect name is passed in, check that multiple names are all reported correctly etc.

@JackStouffer JackStouffer changed the title Fix Issue 17231: Function to get name from Tid Fix Issue 17231 and Improve std.concurrency.locate Mar 7, 2017
@JackStouffer
Copy link
Member Author

register is failing on macOS

@JackStouffer
Copy link
Member Author

which is odd, because I developed this patch on macOS

@JackStouffer
Copy link
Member Author

I don't really have time to fix this right now, so closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants