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

[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