[clang-tidy] Be more liberal about literal zeroes in abseil checks

[hwright]

Summary

Previously, we'd only match on literal floating or integral zeroes, but I've now also learned that some users spell that value as int{0} or float{0}, which also need to be matched.

[Eugene.Zelenko]

test/clang-tidy/abseil-duration-factory-scale.cpp

3 ↗

(On Diff #179306)

cstdint, please.

[aaron.ballman]

clang-tidy/abseil/DurationRewriter.cpp

110 ↗

(On Diff #179306)

Spurious parens can be removed.

test/clang-tidy/abseil-duration-factory-scale.cpp

3 ↗

(On Diff #179306)

Rather than write this as an include, I'd rather just do: namespace std { typedef long long int64_t; } using int64_t = std::int64_t; Then there's no question about whether this is using the host stdint.h or the current-clang-stdint.h.

34 ↗

(On Diff #179306)

Do you also have users doing something like: absl::Seconds(int{});?

[hwright]

Add documentation, adjust test.

[hwright]

btw, I think hasInit should probably be moved into the core set of matchers at some point.

clang-tidy/abseil/DurationRewriter.cpp

110 ↗

(On Diff #179306)

Done. (Though there are similar parens in the implementation of hasArgument. :)

test/clang-tidy/abseil-duration-factory-scale.cpp

34 ↗

(On Diff #179306)

I have not yet seen that.

[aaron.ballman]

LGTM!

[alexfh]

One random late comment.

clang-tools-extra/trunk/test/clang-tidy/abseil-duration-factory-scale.cpp

38

I'd add more context to the CHECK-FIXES patterns, otherwise they are not very reliable. There's no implicit connection to the line numbers, so any long enough subsequence of lines containing 'absl::ZeroDuration()' would satisfy the test. One way to do that is to declare variables with unique names. Another is to add unique comments.

clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp

 
 void DurationFactoryScaleCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       callExpr(
           callee(functionDecl(DurationFactoryFunction()).bind("call_decl")),
           hasArgument(
               0,
               ignoringImpCasts(anyOf(
+                  cxxFunctionalCastExpr(
+                      hasDestinationType(
+                          anyOf(isInteger(), realFloatingPointType())),
+                      hasSourceExpression(initListExpr())),
                   integerLiteral(equals(0)), floatLiteral(equals(0.0)),
                   binaryOperator(hasOperatorName("*"),
                                  hasEitherOperand(ignoringImpCasts(
                                      anyOf(integerLiteral(), floatLiteral()))))
                       .bind("mult_binop"),
                   binaryOperator(hasOperatorName("/"), hasRHS(floatLiteral()))
                       .bind("div_binop")))))
           .bind("call"),

clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp

llvm::StringRef getFactoryForScale(DurationScale Scale) {
   case DurationScale::Microseconds:
     return "absl::Microseconds";
   case DurationScale::Nanoseconds:
     return "absl::Nanoseconds";
   }
   llvm_unreachable("unknown scaling factor");
 }
 
+/// Matches the n'th item of an initializer list expression.
+///
+/// Example matches y.
+///     (matcher = initListExpr(hasInit(0, expr())))
+/// \code
+///   int x{y}.
+/// \endcode
+AST_MATCHER_P2(InitListExpr, hasInit, unsigned, N,
+               ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
+  return N < Node.getNumInits() &&
+          InnerMatcher.matches(*Node.getInit(N)->IgnoreParenImpCasts(), Finder,
+                               Builder);
+}
+
 /// Returns `true` if `Node` is a value which evaluates to a literal `0`.
 bool IsLiteralZero(const MatchFinder::MatchResult &Result, const Expr &Node) {
-  return selectFirst<const clang::Expr>(
-             "val",
-             match(expr(ignoringImpCasts(anyOf(integerLiteral(equals(0)),
-                                               floatLiteral(equals(0.0)))))
-                       .bind("val"),
-                   Node, *Result.Context)) != nullptr;
+  auto ZeroMatcher =
+      anyOf(integerLiteral(equals(0)), floatLiteral(equals(0.0)));
+
+  // Check to see if we're using a zero directly.
+  if (selectFirst<const clang::Expr>(
+          "val", match(expr(ignoringImpCasts(ZeroMatcher)).bind("val"), Node,
+                       *Result.Context)) != nullptr)
+    return true;
+
+  // Now check to see if we're using a functional cast with a scalar
+  // initializer expression, e.g. `int{0}`.
+  if (selectFirst<const clang::Expr>(
+          match(cxxFunctionalCastExpr(
+                    hasDestinationType(
+                        anyOf(isInteger(), realFloatingPointType())),
+                    hasSourceExpression(initListExpr(hasInit(0, ZeroMatcher))))
+                Node, *Result.Context)) != nullptr)
+    return true;
+
+  return false;
 }
 
 llvm::Optional<std::string>
 stripFloatCast(const ast_matchers::MatchFinder::MatchResult &Result,
                const Expr &Node) {
   if (const Expr *MaybeCastArg = selectFirst<const Expr>(
           "cast_arg",
           match(expr(anyOf(cxxStaticCastExpr(

clang-tools-extra/trunk/test/clang-tidy/abseil-duration-factory-scale.cpp

 // RUN: %check_clang_tidy %s abseil-duration-factory-scale %t -- -- -I%S/Inputs
 
 #include "absl/time/time.h"
 
+namespace std { typedef long long int64_t; }
+using int64_t = std::int64_t;
+
 void ScaleTest() {
   absl::Duration d;
 
   // Zeroes
   d = absl::Hours(0);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
   // CHECK-FIXES: absl::ZeroDuration();
   d = absl::Minutes(0);
void ScaleTest() {
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
   // CHECK-FIXES: absl::ZeroDuration();
   d = absl::Seconds(0.0);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
   // CHECK-FIXES: absl::ZeroDuration();
   d = absl::Seconds(0x0.000001p-126f);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
   // CHECK-FIXES: absl::ZeroDuration();
+  d = absl::Seconds(int{0});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::ZeroDuration();
+  d = absl::Seconds(int64_t{0});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::ZeroDuration();
+  d = absl::Seconds(float{0});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::ZeroDuration();
 
   // Fold seconds into minutes
   d = absl::Seconds(30 * 60);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
   // CHECK-FIXES: absl::Minutes(30);
   d = absl::Seconds(60 * 30);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
   // CHECK-FIXES: absl::Minutes(30);
void ScaleTest() {
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
   // CHECK-FIXES: absl::Microseconds(30);
   d = absl::Seconds(30 / 1e6);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
   // CHECK-FIXES: absl::Microseconds(30);
 
   // None of these should trigger the check
   d = absl::Seconds(60);
+  d = absl::Seconds(int{60});
+  d = absl::Seconds(float{60});
   d = absl::Seconds(60 + 30);
   d = absl::Seconds(60 - 30);
   d = absl::Seconds(50 * 30);
   d = absl::Hours(60 * 60);
   d = absl::Nanoseconds(1e-3 * 30);
   d = absl::Seconds(1000 / 30);
   // We don't support division by integers, which could cause rounding
   d = absl::Seconds(10 / 1000);