[PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )

[Aaron Ballman via cfe-commits]

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