came from discussion w @curiouserrandy @mergeconflict @htuch

@sesmith177 does this work on Windows? Is the DWORD a unique identifier of the thread on Windows?

Please read this:

https://linux.die.net/man/3/pthread_self

And in particular:

POSIX.1 allows an implementation wide freedom in choosing the type used to represent a thread ID; for example, representation using either an arithmetic type or a structure is permitted. Therefore, variables of type pthread_t can't portably be compared using the C equality operator (==); use pthread_equal(3) instead.
Thread identifiers should be considered opaque: any attempt to use a thread ID other than in pthreads calls is nonportable and can lead to unspecified results.

Thread IDs are only guaranteed to be unique within a process. A thread ID may be reused after a terminated thread has been joined, or a detached thread has terminated.

The thread ID returned by pthread_self() is not the same thing as the kernel thread ID returned by a call to gettid(2).

Also, I think copying thread IDs is an operation you could add, but you may be able to just share references or use shared_ptr.

Agreed on test coverage, tbh I'm not sure on what the invariants should be. As well, there's no ThreadIdImplTest; should I add one?

@jmarantz will respond to your first post later, need to read it in more detail to understand it. I did recall it wasn't an issue of copy/moving, but rather if you pull a ThreadId out of the ThreadFactory that gets created as a temporary object which was causing problems. I suppose I could wrap that in a smart pointer and store it, but seems odd to me when I only need a unique integer representation. (My implementation just uses the debugString() right now.)

Basically it says "you can't assume anything about the rep for a thread ID". Only valid operation is the functional compare for equality.

@jmarantz : The requirement this PR is responding to is "We need a unique per-thread integer identifier for lookup purposes". Given that, I'm inclined to just change the name of the method in response to your concerns. Would uniqueId() satisfy those concerns? Then figuring out how to create a unique id could be done on a per-implementation basis. (In all implementations I'm aware of so far it would just be a cast.)

@mergeconflict : My only requirements were "per-thread unique", and I'm not sure how to test that other than creating a lot of threads and making sure there aren't any collisions, which as far as tests go is a bit ... lacking :-}. Any other ideas?

How about using debugString() as the map key, or if that doesn't feel right, add new 'std::string mapKey()' method whose impl in posix may just return debugString()?

You could add in the mapKey() API description a guarantee that the resulting key be unique.

This effort was started because I strongly objected to relying on debugString() for the uniqueness property (or, indeed, for any property that needed to be true in production code). I would have no objection to the mapKey() method to solve this problem.

@jmarantz : The requirement this PR is responding to is "We need a unique per-thread integer identifier for lookup purposes". Given that, I'm inclined to just change the name of the method in response to your concerns. Would uniqueId() satisfy those concerns? Then figuring out how to create a unique id could be done on a per-implementation basis. (In all implementations I'm aware of so far it would just be a cast.)

@mergeconflict : My only requirements were "per-thread unique", and I'm not sure how to test that other than creating a lot of threads and making sure there aren't any collisions, which as far as tests go is a bit ... lacking :-}. Any other ideas?

How about declaring ThreadId::operator== and std::hash<ThreadId>?

@mergeconflict I like it. That sounds more efficient than returning a string key.

Yep, that sounds good to me as well.

+1

Only minor annoyance I can see is that you'll need a dynamic_cast for the address of the 'that' arg for operator==. But it's reasonable to just return 'false' if the dynamic_cast returns null.

Ummmm defining specializations for std::hash seems to be prohibited by Google C++, if I'm reading this correctly. Instead I'll define absl::Hash which is apparently safer.

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

What's the status of this?

For the record I vote for not using a specialization for std::hash or absl::Hash, but instead defining a hash/compare functor, passed as template parameters 3 and 4 to absl::flat_hash_map. See me if this is not clear. I think this should be very straightforward to do.

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

My bad, this is still on my radar but I got pulled into other things! Will post another PR with the right absl::hash implementation soon.

You don't need another PR -- I just re-opened this one so there is comment continuity. I think this is pretty simple; you just need a using declaration pointing to a hash() method you add to ThreadId.

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!