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

[Gábor Horváth via cfe-commits]

xazax.hun 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:
> > > 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.
> > 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).
> 
> It would be a known false-positive for a check designed to conform to a particular coding standard. When deviations have come up in the past for various coding standards, we've added an option to enable the additional functionality, which I don't think would be reasonable in this case. Alternatively, the CERT guideline could be modified, but that is unlikely to occur because binding the function pointer is not a security concern (only calling the function).
In case you let binding to function pointer you introduce potential false negatives which is worse in this case in my opinion. 


Repository:
  rL LLVM

https://reviews.llvm.org/D22346