[PATCH] Random Number Generator Refactoring (removing from Module)

JF Bastien jfb at chromium.org
Wed Jul 9 09:22:59 PDT 2014


================
Comment at: include/llvm/IR/Module.h:266
@@ -263,1 +265,3 @@
+  /// versions when the pass does not change.
+  RandomNumberGenerator *createRNG(StringRef PassSalt) const;
 
----------------
I think it would be more usable for LLVM developers, and lead to correct usage, if this took a `Pass &` instead of a `StringRef`.

================
Comment at: include/llvm/Support/RandomNumberGenerator.h:34
@@ -39,1 +33,3 @@
+  /// This constructor should not be used directly. Instead use
+  /// Module::createRNG to create a new RNG salted with the Module ID.
   RandomNumberGenerator(StringRef Salt);
----------------
I'm not sure how general we want this Support RNG to be, versus how paternalistic we want to be in preventing LLVM developers from misusing the RNG. Having a comment here may be the right balance (so people can reuse the RNG for something else if they want), but I'd at consider making the ctor private and adding `friend class Module` if we want to ensure that the RNG is only initialized the "right" way.

================
Comment at: lib/IR/Module.cpp:72
@@ +71,3 @@
+  // might be problematic if the input filename extension changes
+  // (e.g. from .c to .bc or .ll).
+  //
----------------
I'd still like a clarification on this last sentence. I'm not sure it's a problem.

================
Comment at: lib/Support/RandomNumberGenerator.cpp:41
@@ -39,1 +40,3 @@
+  // are using a 64-bit RNG. This isn't a problem since the Mersenne
+  // twister constructor copies these correctly into its initial state
   std::vector<uint32_t> Data;
----------------
Missing a period at the end.

================
Comment at: lib/Support/RandomNumberGenerator.cpp:48
@@ -53,1 +47,3 @@
+  std::vector<uint32_t>::iterator I = Data.end();
+  I = std::copy(Salt.begin(), Salt.end(), I);
 
----------------
Can you do away with I here, pass it straight to `std::copy`? Or use `auto`?

http://reviews.llvm.org/D4377






More information about the llvm-commits mailing list