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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

Thank you for the new check! A few comments.



================
Comment at: clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp:22
+AST_MATCHER_P(Type, isBuiltinType, BuiltinType::Kind, Kind) {
+  if (const BuiltinType *BT = dyn_cast<BuiltinType>(&Node)) {
+    return BT->getKind() == Kind;
----------------
`const auto *` is preferred to avoid duplication of the type name.


================
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"),
----------------
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? 


================
Comment at: clang-tools-extra/clang-tidy/performance/TypePromotionInMathFnCheck.cpp:145
+      // Skip the "::" following the qualifier.
+      FnNameStart = D->getQualifierLoc().getEndLoc().getLocWithOffset(2);
+    }
----------------
`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.


================
Comment at: clang-tools-extra/test/clang-tidy/performance-type-promotion-in-math-fn.cpp:70
+  acosh(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: call to 'acosh' promotes float to double [performance-type-promotion-in-math-fn]
+  // CHECK-FIXES: {{^}}  acoshf(a);{{$}}
----------------
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.


https://reviews.llvm.org/D27284