jlebar added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp:62-67
+ 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:
clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp:145
+ // 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
thing.
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.
https://reviews.llvm.org/D27284
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
