jlebar added inline comments.

Comment at: 
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(TwoDoubleArgFns, parameterCountIs(2),
+                                   hasBuiltinTyParam(0, DoubleTy),
+                                   hasBuiltinTyParam(1, DoubleTy))),
+               hasBuiltinTyArg(0, FloatTy), hasBuiltinTyArg(1, FloatTy))
+          .bind("call"),
alexfh wrote:
> I guess, functions with arbitrary number of parameters can all be handled 
> using `forEachArgumentWithParam`. Roughly like this:
>   forEachArgumentWithParam(
>      hasType(isBuiltinType(FloatTy)),
>      parmVarDecl(hasType(isBuiltinType(DoubleTy)))
> One difference to your existing implementation will be that it will match 
> calls where at least one parameter is double and argument is float, not all 
> of them. Do you expect this to make the check more noisy? 
> Do you expect this to make the check more noisy?

Yes, to the point of not being worth doing, I think.

Specifically, I think there is nothing wrong with calling a two-arg function 
double function with one float and one double arg and expecting the float arg 
to be promoted: `::hypot(3.f, 4.)` probably *should* call the double version.

So checking that the arg types are all `float` is important, I think.  Checking 
that the parameter types are all `double` is less important but also worth 
doing, I think, so if you declare some bizarre function in the global namespace 
called e.g. `::hypot`, we won't suggest changing that to `::hypotf`.

Comment at: 
+      // Skip the "::" following the qualifier.
+      FnNameStart = D->getQualifierLoc().getEndLoc().getLocWithOffset(2);
+    }
alexfh wrote:
> `getLocWithOffset` makes the code quite brittle. Imagine whitespace around 
> `::`, for example. Same below. In order to make this kind of code more 
> robust, you can operate on tokens (using Lexer).
> Same below.
Hm.  I agree this is super-brittle.  But I am having a lot of difficulty using 
the Lexer successfully.

For one thing, there may be comments basically anywhere in this stream, and I 
have to skip over them.  I see getPreviousNonCommentToken, but it doesn't quite 
work if I need to go *forward* in the stream.  I looked at a bunch of other 
uses of the Lexer in clang-tidy and they all looked pretty different from what 
I'm trying to do here, which also suggested that maybe I'm doing the wrong 

Is there no way to get the source location of "sin" out of the DeclRefExpr for 
"::sin"?  I don't see it, but it seems bizarre that it wouldn't be there.

Any tips would be much appreciated.


cfe-commits mailing list

Reply via email to