aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.
Thank you for working on this check!
================
Comment at: clang-tidy/cert/CERTTidyModule.cpp:37
@@ -35,1 +36,3 @@
// DCL
+ CheckFactories.registerCheck<LimitedRandomnessCheck>(
+ "cert-msc50-cpp");
----------------
Please place this under a // MSC heading rather than // DCL.
This check should additionally be listed as cert-msc30-c
(https://www.securecoding.cert.org/confluence/display/c/MSC30-C.+Do+not+use+the+rand%28%29+function+for+generating+pseudorandom+numbers).
================
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+ Finder->addMatcher(
+ declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+ parameterCountIs(0))))
+ .bind("randomGenerator"),
----------------
This should be looking at a callExpr() rather than a declRefExpr(), should it
not?
================
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:31
@@ +30,3 @@
+ Result.Nodes.getNodeAs<DeclRefExpr>("randomGenerator");
+ diag(MatchedDecl->getLocation(), "rand() function has limited randomness, "
+ "use C++11 random library instead");
----------------
Prazek wrote:
> I am not sure about the diagnostics convention, it is possible that you
> should replace comma with semicolon.
This diagnostic is incorrect for C code. Should only suggest C++ <random> in
C++ mode. Also, it should be a semicolon rather than a comma in the diagnostic
text.
================
Comment at: clang-tidy/cert/LimitedRandomnessCheck.h:19
@@ +18,3 @@
+
+/// Pseudorandom number generators are not genuinely random. This checker warns
+/// for the usage of std::rand() function.
----------------
Both sentences are correct, but it doesn't clarify why `std::rand()` is bad.
Should add a sentence between these two that says something about why
std::rand() in particular is called out rather than the other pseudorandom
number generators.
================
Comment at: docs/clang-tidy/checks/cert-msc50-cpp.rst:6
@@ +5,2 @@
+
+Pseudorandom number generators use mathematical algorithms to produce a
sequence of numbers with good statistical properties, but the numbers produced
are not genuinely random. This checker warns for the usage of std::rand().
----------------
Eugene.Zelenko wrote:
> Please use check and highlight std::rand() with ``.
The same argument could be used to disallow C++ <random> generators that aren't
true truly random sources. Should specify more about why rand() is particularly
bad.
================
Comment at: docs/clang-tidy/checks/list.rst:20
@@ -19,2 +19,3 @@
cert-flp30-c
+ cert-msc50-cpp
cert-oop11-cpp (redirects to misc-move-constructor-init) <cert-oop11-cpp>
----------------
Please also add a cert-msc30-c file with a redirect (like fio38-c from above).
================
Comment at: test/clang-tidy/cert-limited-randomness.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s cert-msc50-cpp %t
+
----------------
Please also add a test for C since the check works there as well, and will have
different diagnostic text.
Repository:
rL LLVM
https://reviews.llvm.org/D22346
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
