[PATCH] D44143: Create properly seeded random generator check

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: docs/clang-tidy/checks/cert-properly-seeded-random-generator.rst:26
+    std::random_device dev;
+    std::mt19937 engine3(dev()); // Good
+  }
----------------
Seeding MT19937 with a single 32-bit integer is //not// "Good". It makes the seed super easy to brute-force; and for example, `engine3` will never produce 7 or 13 as its first output.
http://www.pcg-random.org/posts/cpp-seeding-surprises.html

This doesn't affect the implementation or usefulness of this clang-tidy check, which is pretty nifty. I merely object to marking this sample code with the comment "Good" in official documentation. It should be marked "Will not warn" at best. Or replace it with something slightly more realistic, e.g.

    int x = atoi(argv[1]);
    std::mt19937 engine3(x);  // Will not warn

As Aaron said above, seeding with the current time is approximately as good an idea, and "will not warn" with the current diagnostic either.

The correct way to seed a PRNG is to initialize the //entire state// with random bits, not just 32 bits of the state. This can be done, but not yet in standard C++: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0205r0.html


================
Comment at: test/clang-tidy/cert-properly-seeded-random-generator.cpp:76
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random number generator must be seeded with a random_device instead of a constant [cert-properly-seeded-random-generator]
+  engine1.seed(seed);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random number generator must be seeded with a random_device instead of a constant [cert-properly-seeded-random-generator]
----------------
Is the diagnostic suppressed if `seed` is a template parameter? (Not that I'd do this. It's just a corner case I thought of.)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44143