aaron.ballman added a comment.
Thank you for working on this check!
Do you think it might be possible to also add a check for `cert-msc32-c` that
handles the C side of things, and use a common check to implement both? C won't
ever have the C++'isms, but C++ can certainly abuse `std::srand()` so it seems
like there's at least some overlap.
================
Comment at: clang-tidy/cert/CERTTidyModule.cpp:43
+ CheckFactories.registerCheck<ProperlySeededRandomGeneratorCheck>(
+ "cert-properly-seeded-random-generator");
CheckFactories.registerCheck<VariadicFunctionDefCheck>("cert-dcl50-cpp");
----------------
The name should be `cert-msc51-cpp` (and categorized with the other msc checks).
================
Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp:22-24
+ hasAnyName("linear_congruential_engine", "mersenne_twister_engine",
+ "subtract_with_carry_engine", "discard_block_engine",
+ "independent_bits_engine", "shuffle_order_engine"));
----------------
These should be using fully-qualified names, like
`::std::mersenne_twister_engine`. Also, I think this list should be
configurable so that users can add their own engines if needed.
================
Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp:66
+
+ template<class T>
+void ProperlySeededRandomGeneratorCheck::checkSeed(
----------------
Formatting looks off here.
================
Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp:71
+ diag(Func->getExprLoc(), "random number generator must be seeded with "
+ "a random_device instead of default argument");
+ return;
----------------
The diagnostics here aren't quite correct -- a `random_device` isn't
*required*. For instance, it's also fine to open up /dev/random and read a
seed's worth of bytes. I think a better phrasing might be: `random number
generator seeded with a default argument will generate a predictable sequence
of values`.
================
Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp:77-78
+ if (Func->getArg(0)->EvaluateAsInt(Value, *Result.Context)) {
+ diag(Func->getExprLoc(), "random number generator must be seeded with "
+ "a random_device instead of a constant");
+ return;
----------------
I'd probably use similar wording here: `random number generator seeded with a
constant value will generate a predictable sequence of values`.
You can combine these diagnostics and use %select{}0 to choose between the
variants.
================
Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.h:33-34
+ template<class T>
+ void checkSeed(const ast_matchers::MatchFinder::MatchResult &Result,
+ const T *Func);
+};
----------------
It seems like this should be a private function rather than public.
================
Comment at: docs/clang-tidy/checks/cert-properly-seeded-random-generator.rst:3
+
+cert-properly-seeded-random-generator
+=====================================
----------------
When renaming the check, be sure to update titles, file names, etc.
================
Comment at: docs/clang-tidy/checks/cert-properly-seeded-random-generator.rst:7
+This check flags all pseudo-random number engines and engine adaptors
+instantiations when it initialized or seeded with default argument or constant
+expression. Pseudo-random number engines seeded with a predictable value may
----------------
Remove "it".
================
Comment at:
docs/clang-tidy/checks/cert-properly-seeded-random-generator.rst:22-23
+
+ std::time_t t;
+ engine1.seed(std::time(&t)); // Bad, system time might be controlled by
user
+
----------------
There's no test case for this test, and I don't think this code would generate
a diagnostic in the current check form. This might require a
(user-configurable) list of disallowed sources of seed values. For instance, it
should diagnose when called with a `time_t` value or a direct call to a time
function, but it need not diagnose if the value is somehow obscured. e.g.,
```
unsigned get_seed() { std::time_t t; return std::time(&t); }
engine1.seed(get_seed()); // No diagnostic required
```
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44143
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
