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

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.

================
Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+  Finder->addMatcher(
+      declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+                                              parameterCountIs(0))))
+          .bind("randomGenerator"),
----------------
aaron.ballman wrote:
> Prazek wrote:
> > aaron.ballman wrote:
> > > This should be looking at a callExpr() rather than a declRefExpr(), should it not?
> > I was also thinking about this, but this is actually better, because it will also match with binding rand with function pointer.
> True, but a DeclRefExpr doesn't mean it's a function call. Binding the function is not contrary to the CERT rule, just calling it. For instance, the following pathological case will be caught by this check:
> ```
> if (std::rand) {}
> ```
> The behavior of this check should be consistent with cert-env33-c, which only looks at calls. (If we really care about bound functions, we'd need flow control analysis, and I think that's overkill for both of those checks, but wouldn't be opposed to someone writing the flow analysis if they really wanted to.)
It would indeed fire on this pathological case, but I don't think we should care about cases like this, because no one is writing code like this (and if he would then it would probably be a bug).
I don't think that there is much code that binds pointer to std::rand either, but I think it would be good to display warning for this, because even if the function would be never called, then it means that this is a bug, and if it would be called then it would be nice to tell user that rand might be used here.

Anyway I don't oppose for changing it to callExpr, but I think it is better this way.


Repository:
  rL LLVM

https://reviews.llvm.org/D22346