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

Reply via email to