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

[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