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-unsubscr...@qpid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
