You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/11/03 13:22:50 UTC

[GitHub] [qpid-proton] DreamPearl opened a new pull request #340: PROTON-2396: [WIP] Use random_device for seed initialization in uuid.cpp

DreamPearl opened a new pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340


   [PROTON-2396](https://issues.apache.org/jira/browse/PROTON-2396?jql=resolution%20%3D%20Unresolved%20AND%20assignee%20%3D%20currentUser()%20AND%20project%20%3D%2012313720)


-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #340: PROTON-2396: [WIP] Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r746898809



##########
File path: cpp/src/uuid.cpp
##########
@@ -42,15 +44,17 @@ namespace {
 
 // Seed the random number generated once at startup.
 struct seed {

Review comment:
       I use thread_local here, this is my first time using it, and I couldn't find many examples that include both cpp and hpp files. Can you please take a look?




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] DreamPearl commented on pull request #340: PROTON-2396: Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
DreamPearl commented on pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#issuecomment-971547101


   Now the build is passing. @astitcher Can you please take a look?


-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #340: PROTON-2396: Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r756923139



##########
File path: cpp/include/proton/uuid.hpp
##########
@@ -35,13 +38,20 @@ namespace proton {
 
 /// A 16-byte universally unique identifier.
 class uuid : public byte_array<16> {
+
+  private:
+    thread_local static std::independent_bits_engine<std::mt19937, CHAR_BIT,
+                                                     unsigned int>
+        engine;
+    thread_local static std::seed_seq seed;
+

Review comment:
       Moved them out of the class and put them all in struct construct.




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #340: PROTON-2396: Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r756924868



##########
File path: cpp/src/uuid.cpp
##########
@@ -70,12 +72,7 @@ uuid uuid::copy(const char* bytes) {
 
 uuid uuid::random() {
     uuid bytes;
-    int r = std::rand();
-    for (size_t i = 0; i < bytes.size(); ++i ) {
-        bytes[i] = r & 0xFF;
-        r >>= 8;
-        if (!r) r = std::rand();
-    }
+    std::generate(bytes.begin(), bytes.end(), std::ref(engine));

Review comment:
       I use seed_.engine.




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #340: PROTON-2396: [WIP] Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r746890792



##########
File path: cpp/src/uuid.cpp
##########
@@ -70,11 +74,11 @@ uuid uuid::copy(const char* bytes) {
 
 uuid uuid::random() {
     uuid bytes;
-    int r = std::rand();
+    int r = seed_.dist(seed_.engine);
     for (size_t i = 0; i < bytes.size(); ++i ) {
         bytes[i] = r & 0xFF;
         r >>= 8;
-        if (!r) r = std::rand();
+        if (!r) r = seed_.dist(seed_.engine);

Review comment:
       Done, Thanks!




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] jiridanek commented on a change in pull request #340: PROTON-2396: Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r751541924



##########
File path: cpp/include/proton/uuid.hpp
##########
@@ -35,13 +38,20 @@ namespace proton {
 
 /// A 16-byte universally unique identifier.
 class uuid : public byte_array<16> {
+
+  private:
+    thread_local static std::independent_bits_engine<std::mt19937, CHAR_BIT,
+                                                     unsigned int>
+        engine;
+    thread_local static std::seed_seq seed;
+

Review comment:
       @DreamPearl Right, ABI. Sorry I forgot to think about that before.




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] jiridanek commented on a change in pull request #340: PROTON-2396: [WIP] Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r744660582



##########
File path: cpp/src/uuid.cpp
##########
@@ -42,15 +44,17 @@ namespace {
 
 // Seed the random number generated once at startup.
 struct seed {

Review comment:
       (assuming that there is not something else which prevents concurrent use; I think there isn'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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] codecov-commenter edited a comment on pull request #340: PROTON-2396: Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#issuecomment-978330608


   # [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/340?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#340](https://codecov.io/gh/apache/qpid-proton/pull/340?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ab304d5) into [main](https://codecov.io/gh/apache/qpid-proton/commit/ba4a773c6c5d4ceeb5b680ab57d395a1287bdc2a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ba4a773) will **increase** coverage by `19.83%`.
   > The diff coverage is `n/a`.
   
   > :exclamation: Current head ab304d5 differs from pull request most recent head b7ffe28. Consider uploading reports for the commit b7ffe28 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-proton/pull/340/graphs/tree.svg?width=650&height=150&src=pr&token=UKKzV9XnFF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/qpid-proton/pull/340?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff             @@
   ##             main     #340       +/-   ##
   ===========================================
   + Coverage   68.22%   88.05%   +19.83%     
   ===========================================
     Files         356       47      -309     
     Lines       73086     2394    -70692     
   ===========================================
   - Hits        49861     2108    -47753     
   + Misses      23225      286    -22939     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/qpid-proton/pull/340?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [ruby/lib/core/uri.rb](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVieS9saWIvY29yZS91cmkucmI=) | `74.07% <0.00%> (-25.93%)` | :arrow_down: |
   | [cpp/include/proton/uuid.hpp](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL2luY2x1ZGUvcHJvdG9uL3V1aWQuaHBw) | | |
   | [cpp/src/uuid.cpp](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL3NyYy91dWlkLmNwcA==) | | |
   | [cpp/include/proton/map.hpp](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL2luY2x1ZGUvcHJvdG9uL21hcC5ocHA=) | | |
   | [cpp/include/proton/scalar.hpp](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL2luY2x1ZGUvcHJvdG9uL3NjYWxhci5ocHA=) | | |
   | [c/src/core/message.c](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9zcmMvY29yZS9tZXNzYWdlLmM=) | | |
   | [cpp/include/proton/sasl.hpp](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL2luY2x1ZGUvcHJvdG9uL3Nhc2wuaHBw) | | |
   | [c/src/sasl/default\_sasl.c](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9zcmMvc2FzbC9kZWZhdWx0X3Nhc2wuYw==) | | |
   | [cpp/examples/client.cpp](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL2V4YW1wbGVzL2NsaWVudC5jcHA=) | | |
   | [cpp/src/reconnect\_options\_impl.hpp](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL3NyYy9yZWNvbm5lY3Rfb3B0aW9uc19pbXBsLmhwcA==) | | |
   | ... and [300 more](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-proton/pull/340?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/340?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ba4a773...b7ffe28](https://codecov.io/gh/apache/qpid-proton/pull/340?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] jiridanek commented on pull request #340: PROTON-2396: [WIP] Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
jiridanek commented on pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#issuecomment-965502114


   Eh, I wanted to be putting comments on individual lines of code. They do show correctly at https://github.com/apache/qpid-proton/commit/a3c7f46498baaf7e2a4005dbbd2d02201121e745, but they do not show with the lines here on the PR. Sorry. Anyways, yeah, I think I have pretty much nothing to complain about here.


-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] asfgit closed pull request #340: PROTON-2396: Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340


   


-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #340: PROTON-2396: Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r756929816



##########
File path: cpp/src/uuid.cpp
##########
@@ -38,20 +43,17 @@
 namespace proton {
 
 namespace {
-
-
-// Seed the random number generated once at startup.
-struct seed {
-    seed() {
-        // A hash of time and PID, time alone is a bad seed as programs started
-        // within the same second will get the same seed.
-        unsigned long secs = time(0);
-        unsigned long pid = GETPID();
-        std::srand(((secs*181)*((pid-83)*359))%104729);
-    }
-} seed_;
-
-}
+// A hash of time, PID and random_device, time alone is a bad seed as programs
+// started within the same second will get the same seed.

Review comment:
       Yeah, my bad! It was in nanoseconds. Updated the comment.




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] jiridanek commented on a change in pull request #340: PROTON-2396: [WIP] Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r744535498



##########
File path: cpp/src/uuid.cpp
##########
@@ -42,15 +44,17 @@ namespace {
 
 // Seed the random number generated once at startup.
 struct seed {

Review comment:
       Instead of having the seed_, the variables could be private static in the uuid class, btw.

##########
File path: cpp/src/uuid.cpp
##########
@@ -42,15 +44,17 @@ namespace {
 
 // Seed the random number generated once at startup.
 struct seed {

Review comment:
       Instead of having the seed_ separately, the variables could be private static in the uuid class, btw.




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] astitcher commented on pull request #340: PROTON-2396: Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
astitcher commented on pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#issuecomment-982155262


   Commited with a minor change to simplify the initialization/use of the the random generator


-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] jiridanek commented on a change in pull request #340: PROTON-2396: [WIP] Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r747314538



##########
File path: cpp/include/proton/uuid.hpp
##########
@@ -35,13 +38,20 @@ namespace proton {
 
 /// A 16-byte universally unique identifier.
 class uuid : public byte_array<16> {
+
+  private:
+    thread_local static std::independent_bits_engine<std::mt19937, CHAR_BIT,

Review comment:
       Looks correct to me. TBH, I never myself used thread_local in c++ either ;~




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #340: PROTON-2396: Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r756079211



##########
File path: cpp/src/uuid.cpp
##########
@@ -38,20 +43,17 @@
 namespace proton {
 
 namespace {
-
-
-// Seed the random number generated once at startup.
-struct seed {
-    seed() {
-        // A hash of time and PID, time alone is a bad seed as programs started
-        // within the same second will get the same seed.
-        unsigned long secs = time(0);
-        unsigned long pid = GETPID();
-        std::srand(((secs*181)*((pid-83)*359))%104729);
-    }
-} seed_;
-
-}
+// A hash of time, PID and random_device, time alone is a bad seed as programs
+// started within the same second will get the same seed.
+thread_local unsigned long ticks =
+    std::chrono::system_clock::now().time_since_epoch().count();
+unsigned long pid = GETPID();
+unsigned int rd = std::random_device{}();
+} // namespace
+
+thread_local std::seed_seq uuid::seed{ticks, pid, (unsigned long)rd};
+thread_local std::independent_bits_engine<std::mt19937, CHAR_BIT, unsigned int>
+    uuid::engine(seed);

Review comment:
       I have modified the code to initialize the seed once using the struct seed construct.




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] DreamPearl commented on a change in pull request #340: PROTON-2396: Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
DreamPearl commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r756068571



##########
File path: cpp/include/proton/uuid.hpp
##########
@@ -35,13 +38,20 @@ namespace proton {
 
 /// A 16-byte universally unique identifier.
 class uuid : public byte_array<16> {
+
+  private:
+    thread_local static std::independent_bits_engine<std::mt19937, CHAR_BIT,
+                                                     unsigned int>
+        engine;
+    thread_local static std::seed_seq seed;
+
   public:
     /// Make a copy.
     PN_CPP_EXTERN static uuid copy();
 
     /// Return a uuid copied from bytes.  Bytes must point to at least
     /// 16 bytes.  If `bytes == 0` the UUID is zero-initialized.
-    PN_CPP_EXTERN static uuid copy(const char* bytes);
+    PN_CPP_EXTERN static uuid copy(const char *bytes);

Review comment:
       Yep, I am still using git clang-format. Updated the local configuration to adapt this.
   Thanks!




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] astitcher commented on a change in pull request #340: PROTON-2396: Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r751379226



##########
File path: cpp/include/proton/uuid.hpp
##########
@@ -35,13 +38,20 @@ namespace proton {
 
 /// A 16-byte universally unique identifier.
 class uuid : public byte_array<16> {
+
+  private:
+    thread_local static std::independent_bits_engine<std::mt19937, CHAR_BIT,
+                                                     unsigned int>
+        engine;
+    thread_local static std::seed_seq seed;
+

Review comment:
       Please don't add these private members to the class. It changes the ABI (also the API but in a more subtle way). 
   Since these are thread_locals anyway they don't need to be per instance variables so can just be static to the implementation file itself and not be in the class at all.

##########
File path: cpp/src/uuid.cpp
##########
@@ -38,20 +43,17 @@
 namespace proton {
 
 namespace {
-
-
-// Seed the random number generated once at startup.
-struct seed {
-    seed() {
-        // A hash of time and PID, time alone is a bad seed as programs started
-        // within the same second will get the same seed.
-        unsigned long secs = time(0);
-        unsigned long pid = GETPID();
-        std::srand(((secs*181)*((pid-83)*359))%104729);
-    }
-} seed_;
-
-}
+// A hash of time, PID and random_device, time alone is a bad seed as programs
+// started within the same second will get the same seed.
+thread_local unsigned long ticks =
+    std::chrono::system_clock::now().time_since_epoch().count();
+unsigned long pid = GETPID();
+unsigned int rd = std::random_device{}();
+} // namespace
+
+thread_local std::seed_seq uuid::seed{ticks, pid, (unsigned long)rd};
+thread_local std::independent_bits_engine<std::mt19937, CHAR_BIT, unsigned int>
+    uuid::engine(seed);

Review comment:
       I like the struct seed construct to initialise the seed once. You can shift your code into there and make it thread_local I think which will be a neater implementation and a smaller change.

##########
File path: cpp/include/proton/uuid.hpp
##########
@@ -35,13 +38,20 @@ namespace proton {
 
 /// A 16-byte universally unique identifier.
 class uuid : public byte_array<16> {
+
+  private:
+    thread_local static std::independent_bits_engine<std::mt19937, CHAR_BIT,
+                                                     unsigned int>
+        engine;
+    thread_local static std::seed_seq seed;
+
   public:
     /// Make a copy.
     PN_CPP_EXTERN static uuid copy();
 
     /// Return a uuid copied from bytes.  Bytes must point to at least
     /// 16 bytes.  If `bytes == 0` the UUID is zero-initialized.
-    PN_CPP_EXTERN static uuid copy(const char* bytes);
+    PN_CPP_EXTERN static uuid copy(const char *bytes);

Review comment:
       FWIW the convention used in our C++ is in line with usual C++ conventions and has the '*' in the type side not the name side. Our C code is in line with more usual C conventions and is the other way round. This is probably not written down anywhere - sorry!

##########
File path: cpp/src/uuid.cpp
##########
@@ -70,12 +72,7 @@ uuid uuid::copy(const char* bytes) {
 
 uuid uuid::random() {
     uuid bytes;
-    int r = std::rand();
-    for (size_t i = 0; i < bytes.size(); ++i ) {
-        bytes[i] = r & 0xFF;
-        r >>= 8;
-        if (!r) r = std::rand();
-    }
+    std::generate(bytes.begin(), bytes.end(), std::ref(engine));

Review comment:
       in my way of doing this engine would be seed_::engine - maybe seed_ should have a better name?

##########
File path: cpp/src/uuid.cpp
##########
@@ -38,20 +43,17 @@
 namespace proton {
 
 namespace {
-
-
-// Seed the random number generated once at startup.
-struct seed {
-    seed() {
-        // A hash of time and PID, time alone is a bad seed as programs started
-        // within the same second will get the same seed.
-        unsigned long secs = time(0);
-        unsigned long pid = GETPID();
-        std::srand(((secs*181)*((pid-83)*359))%104729);
-    }
-} seed_;
-
-}
+// A hash of time, PID and random_device, time alone is a bad seed as programs
+// started within the same second will get the same seed.

Review comment:
       I think this comment maybe now incorrect as std::chrono::system_clock::now() isn't in seconds any more (is it?)




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] jiridanek commented on a change in pull request #340: PROTON-2396: [WIP] Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r744534838



##########
File path: cpp/src/uuid.cpp
##########
@@ -42,15 +44,17 @@ namespace {
 
 // Seed the random number generated once at startup.
 struct seed {

Review comment:
       Threadsafety, I almost forgot! The std:: random stuff is not threadsafe, so maybe it should be declared thread_local as that would be easiest.




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] jiridanek edited a comment on pull request #340: PROTON-2396: [WIP] Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
jiridanek edited a comment on pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#issuecomment-965502114






-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] jiridanek commented on a change in pull request #340: PROTON-2396: [WIP] Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r744523432



##########
File path: cpp/src/uuid.cpp
##########
@@ -70,11 +74,11 @@ uuid uuid::copy(const char* bytes) {
 
 uuid uuid::random() {
     uuid bytes;
-    int r = std::rand();
+    int r = seed_.dist(seed_.engine);
     for (size_t i = 0; i < bytes.size(); ++i ) {
         bytes[i] = r & 0xFF;
         r >>= 8;
-        if (!r) r = std::rand();
+        if (!r) r = seed_.dist(seed_.engine);

Review comment:
       Thinking about the original algorithm, this line means that if there is a zero byte at the "end" of the random number, it will not be put into the UUID and a new number will be generated instead. For example, if the `seed_.dist` before the loop generated a 0, then only the first byte will be put into the uuid and then immediately a new random number will be generated. I'm trying to say that the random data in the UUID will be IMO somewhat biased against `\x00` bytes.
   
   Maybe best to use something like `std::independent_bits_engine` suggested in the first answer on https://stackoverflow.com/questions/25298585/efficiently-generating-random-bytes-of-data-in-c11-14

##########
File path: cpp/src/uuid.cpp
##########
@@ -70,11 +74,11 @@ uuid uuid::copy(const char* bytes) {
 
 uuid uuid::random() {
     uuid bytes;
-    int r = std::rand();
+    int r = seed_.dist(seed_.engine);

Review comment:
       std::rand returns numbers from 0 to INT_MAX. You are changing the range now to include negative numbers. Is it intentional? Does it matter? AFAIK it does, a little, for the better.

##########
File path: cpp/src/uuid.cpp
##########
@@ -42,15 +44,17 @@ namespace {
 
 // Seed the random number generated once at startup.
 struct seed {
-    seed() {
+    std::random_device device;
+    std::mt19937 engine;
+    std::uniform_int_distribution<int> dist;
+    seed():engine(device()), dist(INT_MIN, INT_MAX) {

Review comment:
       ```suggestion
       std::mt19937 engine(std::random_device{}());
       std::uniform_int_distribution<int> dist(INT_MIN, INT_MAX);
       seed() {
   ```
   
   I did not try the above, so maybe I am doing something completely silly, but to me, if that works, it is a better option. Initializing in the constructor initializer list seems clumsy, in that you have to make sure the order of the declared variables and the order in which they are initialized is the same.
   
   The C++ version of `INT_MAX` seems to be `std::numeric_limits<IntType>::max()`. Probably not worth using, as it is significantly longer string to type.

##########
File path: cpp/src/uuid.cpp
##########
@@ -42,15 +44,17 @@ namespace {
 
 // Seed the random number generated once at startup.
 struct seed {

Review comment:
       I'd rename this struct. At this point it is not a seed, it is the generator.




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] codecov-commenter commented on pull request #340: PROTON-2396: Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#issuecomment-978330608


   # [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/340?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#340](https://codecov.io/gh/apache/qpid-proton/pull/340?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ab304d5) into [main](https://codecov.io/gh/apache/qpid-proton/commit/ba4a773c6c5d4ceeb5b680ab57d395a1287bdc2a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ba4a773) will **increase** coverage by `19.83%`.
   > The diff coverage is `n/a`.
   
   > :exclamation: Current head ab304d5 differs from pull request most recent head b7ffe28. Consider uploading reports for the commit b7ffe28 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-proton/pull/340/graphs/tree.svg?width=650&height=150&src=pr&token=UKKzV9XnFF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/qpid-proton/pull/340?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff             @@
   ##             main     #340       +/-   ##
   ===========================================
   + Coverage   68.22%   88.05%   +19.83%     
   ===========================================
     Files         356       47      -309     
     Lines       73086     2394    -70692     
   ===========================================
   - Hits        49861     2108    -47753     
   + Misses      23225      286    -22939     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/qpid-proton/pull/340?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [ruby/lib/core/uri.rb](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVieS9saWIvY29yZS91cmkucmI=) | `74.07% <0.00%> (-25.93%)` | :arrow_down: |
   | [cpp/include/proton/uuid.hpp](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL2luY2x1ZGUvcHJvdG9uL3V1aWQuaHBw) | | |
   | [cpp/src/uuid.cpp](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL3NyYy91dWlkLmNwcA==) | | |
   | [cpp/src/receiver\_options.cpp](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL3NyYy9yZWNlaXZlcl9vcHRpb25zLmNwcA==) | | |
   | [cpp/src/credit\_test.cpp](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL3NyYy9jcmVkaXRfdGVzdC5jcHA=) | | |
   | [c/src/core/object/map.c](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Yy9zcmMvY29yZS9vYmplY3QvbWFwLmM=) | | |
   | [cpp/src/message\_test.cpp](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL3NyYy9tZXNzYWdlX3Rlc3QuY3Bw) | | |
   | [cpp/include/proton/internal/pn\_unique\_ptr.hpp](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y3BwL2luY2x1ZGUvcHJvdG9uL2ludGVybmFsL3BuX3VuaXF1ZV9wdHIuaHBw) | | |
   | [python/tests/proton-test](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3Rlc3RzL3Byb3Rvbi10ZXN0) | | |
   | [python/proton/\_message.py](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3Byb3Rvbi9fbWVzc2FnZS5weQ==) | | |
   | ... and [300 more](https://codecov.io/gh/apache/qpid-proton/pull/340/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-proton/pull/340?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/340?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ba4a773...b7ffe28](https://codecov.io/gh/apache/qpid-proton/pull/340?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] astitcher commented on a change in pull request #340: PROTON-2396: Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r752828384



##########
File path: cpp/include/proton/uuid.hpp
##########
@@ -35,13 +38,20 @@ namespace proton {
 
 /// A 16-byte universally unique identifier.
 class uuid : public byte_array<16> {
+
+  private:
+    thread_local static std::independent_bits_engine<std::mt19937, CHAR_BIT,
+                                                     unsigned int>
+        engine;
+    thread_local static std::seed_seq seed;
+

Review comment:
       Actually thinking a bit more. It probably doesn't actually change the ABI. But it does unnecessarily leak implementation details of the uuid class. IMO it's better to just make these static thread_local in the implementation file without putting them in the class at all. This will generate pretty much exactly the same code but is better encapsulated (again IMO)




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-proton] jiridanek commented on a change in pull request #340: PROTON-2396: Use random_device for seed initialization in uuid.cpp

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #340:
URL: https://github.com/apache/qpid-proton/pull/340#discussion_r751541523



##########
File path: cpp/include/proton/uuid.hpp
##########
@@ -35,13 +38,20 @@ namespace proton {
 
 /// A 16-byte universally unique identifier.
 class uuid : public byte_array<16> {
+
+  private:
+    thread_local static std::independent_bits_engine<std::mt19937, CHAR_BIT,
+                                                     unsigned int>
+        engine;
+    thread_local static std::seed_seq seed;
+
   public:
     /// Make a copy.
     PN_CPP_EXTERN static uuid copy();
 
     /// Return a uuid copied from bytes.  Bytes must point to at least
     /// 16 bytes.  If `bytes == 0` the UUID is zero-initialized.
-    PN_CPP_EXTERN static uuid copy(const char* bytes);
+    PN_CPP_EXTERN static uuid copy(const char *bytes);

Review comment:
       @DreamPearl Are you still using that clang-format config? You could change the setting there and commit that... some time in the future.




-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org