This checks for calls to double-precision math.h with single-precision
arguments. For example, it suggests replacing ::sin(0.f) with
::sinf(0.f).
Details
Diff Detail
- Build Status
Buildable 1733 Build 1733: arc lint + arc unit
Event Timeline
Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).
clang-tools-extra/docs/clang-tidy/checks/performance-type-promotion-in-math-fn.rst | ||
---|---|---|
6 | Please enclose math.h, float and double into ``. Will be good idea to mention cmath too. | |
8 | Please enclose functions into ``. Same in other places. | |
9 | Please fix double space. |
Formatting fixes in documentation.
clang-tools-extra/docs/clang-tidy/checks/performance-type-promotion-in-math-fn.rst | ||
---|---|---|
6 |
Done. I used two backticks, I hope that's the right thing.
What about cmath, exactly? |
clang-tools-extra/docs/clang-tidy/checks/performance-type-promotion-in-math-fn.rst | ||
---|---|---|
6 | cmath is proper C++ header for math functions prototypes. |
Thank you for the new check! A few comments.
clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp | ||
---|---|---|
23 | const auto * is preferred to avoid duplication of the type name. | |
63–68 | 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? | |
146 | 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. | |
clang-tools-extra/test/clang-tidy/performance-type-promotion-in-math-fn.cpp | ||
71 | We usually truncate repeated static parts of CHECK patterns (except for the first one) to make tests a bit less verbose. You can at least truncate the check name in all but the first check pattern. |
Update test, use "auto". Still need to figure out how to lex these function calls properly.
clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp | ||
---|---|---|
63–68 |
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. | |
146 | 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. |
clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp | ||
---|---|---|
63–68 | Fair enough. I don't like the verbosity of the code here, but it may really be valuable to be strict here. | |
146 | Skipping from :: to the next identifier should be straightforward using Lexer::getRawToken and Lexer::getLocForEndOfToken. Tell me if you need more details. |
Suggest std::foo instead of ::foof.
I spoke with rtrieu IRL and he suggested I do this to match what he did in
SemaChecking.cpp for calls to "abs". Seems reasonable to me. This also lets
me sidestep the question of lexing the qualifiers, which is nice.
@alexfh , friendly ping. I'm not in a huge rush on this or anything, but I don't want one or both of us to lose all context here...
LG with one nit.
clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp | ||
---|---|---|
146–154 | Maybe just make a static StringSet of all the names and check whether the name is in it? | |
168–169 | We definitely should. See the use of IncludeInserter in UnnecessaryValueParamCheck, for example. Fine for a follow-up. |
Indeed, sorry I missed that. Done now, will be reflected when I submit.
clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp | ||
---|---|---|
168–169 | OK, would rather send a followup because this patch is already nontrivial. I'll get to work on that -- thank you for the pointer. |
const auto * is preferred to avoid duplication of the type name.