[PATCH] D27284: [ClangTidy] Add new performance-type-promotion-in-math-fn check.

[Justin Lebar via Phabricator via cfe-commits]

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