[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp

Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/981 The latest build failure was environmental and passed all other tests: ``` make[3]: Leaving directory `/thrift/src/lib/lua' make[3]: write error make[2]: *** [check-recursive] Erro

[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp

Github user ben-craig commented on the issue: https://github.com/apache/thrift/pull/981 Your latest revision look fine, I was mainly trying to address nsuke's post. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your

[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp

Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/981 @ben-craig are you saying additional code changes are needed? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not

[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp

Github user ben-craig commented on the issue: https://github.com/apache/thrift/pull/981 The default op=, and even the default conversion operators from atomic to T are fine. @nsuke I would prefer we keep the access to TThreadPoolServer timeout variables as seq_cst. The sette

[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp

Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/981 Are you suggesting I get rid of the load and store calls and instead just use the provided "operator=" from the atomic? I'm okay with that... it is much more readable. --- If your project is set u

[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp

Github user nsuke commented on the issue: https://github.com/apache/thrift/pull/981 @jeking3 the atomics for TThreadPoolServer fields are for 64-bit field read-write on 32bit-x86 and other platforms ? If so, we only need relaxed. That said, as @ben-craig suggested, leaviing it

[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp

Github user nsuke commented on the issue: https://github.com/apache/thrift/pull/981 Seeing the [commit log](https://github.com/apache/thrift/commit/05b19b57cf2e224ab49e5cf983677361acb18311#diff-487498268b6e5a491d25456332fd44a1R107), it looks like more about not to use `acquire` for `s

[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp

Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/981 I am okay letting things default to something else here. I followed the boost examples and it passed the build with those settings. --- If your project is set up for it, you can reply to this email

[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp

Github user ben-craig commented on the issue: https://github.com/apache/thrift/pull/981 Unless you have extremely compelling profiling results, the only atomic memory order you should be using is memory_order_seq_cst. Even better, you get seq_cst for free as the default parameter of

[GitHub] thrift issue #981: THRIFT-3038: fix up some volatiles in cpp

Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/981 Quite interesting, the job that failed is related to the code changes but the error message comes from a version of boost before 1.53. It looks like that build job isn't making sure boost is at the