westonpace commented on a change in pull request #10644:
URL: https://github.com/apache/arrow/pull/10644#discussion_r663227068
##########
File path: cpp/src/arrow/util/async_generator.h
##########
@@ -1247,6 +1248,8 @@ class BackgroundGenerator {
}
protected:
+ static constexpr uint64_t
kUnlikelyThreadId{std::numeric_limits<uint64_t>::max()};
Review comment:
I couldn't see any reason this wouldn't work but why not just use the
`uint64_t` version of `std::thread::id()`? Is it to allow this to be
`constexpr`?
##########
File path: cpp/src/arrow/util/io_util.cc
##########
@@ -1734,5 +1665,21 @@ int64_t GetRandomSeed() {
return static_cast<int64_t>(seed_gen());
}
+uint64_t GetThreadId() {
Review comment:
So I did a bit of research on this out of curiosity. TL;DR: this should
be safe enough.
`std::thread::id` is often a number, sometimes it is a pointer to a struct,
and sometimes it is a struct
(https://github.com/nginx/unit/blob/master/src/nxt_thread_id.h attempts to
catalogue all the possible methods). I couldn't find any example where it is
larger than 8 bytes. This approach has some precedent. OpenSSL originally
required thread_id be coerced into `unsigned long` and now it has an
implementation where it is either `unsigned long` or a pointer. However, I
think that had to be done because on some systems a pointer may be larger than
`unsigned long`. So I think `uint64_t` is safe since a pointer should never be
larger than 8 bytes.
One potential concern is that technically `==` could be overloaded so that
two threads could in theory have the same `uint64_t` representation but not be
equal. One case is when a thread id is a pointer and that pointer is reused.
However, the spec for std::thread states `Once a thread has finished, the value
of std::thread::id may be reused by another thread` so this is already a given.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
