alexfh requested changes to this revision. alexfh added inline comments. This revision now requires changes to proceed.
================ 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"), ---------------- jlebar wrote: > 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`. Fair enough. I don't like the verbosity of the code here, but it may really be valuable to be strict here. ================ Comment at: clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp:145 + // Skip the "::" following the qualifier. + FnNameStart = D->getQualifierLoc().getEndLoc().getLocWithOffset(2); + } ---------------- jlebar wrote: > 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. Skipping from `::` to the next identifier should be straightforward using Lexer::getRawToken and Lexer::getLocForEndOfToken. Tell me if you need more details. https://reviews.llvm.org/D27284 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits