[GitHub] [arrow] bkietz commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures

[GitBox <gi...@apache.org>]

bkietz commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r661696226



##########
File path: cpp/src/arrow/compute/exec/expression.cc
##########
@@ -294,19 +293,21 @@ size_t Expression::hash() const {
   }
 
   auto call = CallNotNull(*this);
-  if (call->hash != nullptr) {
-    return call->hash->load();
+  const auto cached_hash = call->hash.load();

Review comment:
       Instead, could we simply avoid lazy hashing?
   
   https://github.com/apache/arrow/pull/10397/files?file-filters%5B%5D=.cc&file-filters%5B%5D=.cpp&file-filters%5B%5D=.h&file-filters%5B%5D=.pxd&file-filters%5B%5D=.pyx#diff-1c7baea2c704ebf6ec38232e30aa84900e7ea8f81d6faa0f138c30cd3e4ee7ecR45-R51
   
   This will remove the need to write Call constructors too

##########
File path: cpp/src/arrow/util/async_generator.h
##########
@@ -1338,11 +1342,34 @@ class BackgroundGenerator {
       return next;
     }
 
+#if (defined(__GNUC__) || defined(__clang__))
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wclass-memaccess"
+#endif
+
+    void SetWorkerThreadId(const std::thread::id tid) {
+      uint64_t equiv{0};
+      // std::thread::id is trivially copyable as per C++ spec, so this should work

Review comment:
       ```suggestion
         // std::thread::id is trivially copyable as per C++ spec,
         // so type punning as a uint64_t should work
         static_assert(sizeof(std::thread::id) <= sizeof(uint64_t),
                       "std::thread::id can't fit into uint64_t");
   ```




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org