[PATCH] D16764: Move incorrect-roundings to upstream.

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Nice! See a few comments inline.


================
Comment at: clang-tidy/misc/IncorrectRoundings.cpp:38
@@ +37,3 @@
+  // Match a floating point expression.
+  auto FloatType = expr(anyOf(hasType(qualType(asString("long double"))),
+                              hasType(qualType(asString("double"))),
----------------
We should use a more effective way of checking whether the type is a (long)? double or float. There'a `builtinType()` matcher that we could extend with a narrowing matcher calling `BuiltinType::isFloatingPoint()` (that can be added to ASTMatchers.h, I think).

================
Comment at: clang-tidy/misc/IncorrectRoundings.cpp:50
@@ +49,3 @@
+  auto OneSideHalf = anyOf(
+      allOf(hasLHS(FloatOrCastHalf), hasRHS(FloatType.bind("ExprOnRhs"))),
+      allOf(hasRHS(FloatOrCastHalf), hasLHS(FloatType.bind("ExprOnLhs"))));
----------------
Since these two `.bind()` calls are in alternative branches, they can use the same identifier. This would also simplify the code in the callback.

================
Comment at: clang-tidy/misc/IncorrectRoundings.cpp:71
@@ +70,3 @@
+       "casting (double + 0.5) to integer leads to incorrect rounding; "
+       "consider using lrint (#include <cmath>) instead");
+}
----------------
I don't think we should recommend `lrint`, since its behavior can be changed by [[ http://www.cplusplus.com/reference/cfenv/fesetround | fesetround() ]]. [[ http://www.cplusplus.com/reference/cmath/lround/ | lround() ]] and [[ http://www.cplusplus.com/reference/cmath/llround/ | llround() ]] are better alternatives, IMO. We could also suggest an automated fix for this.


http://reviews.llvm.org/D16764