[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:29-31
+    llvm::APSInt ApInt(/*BitWidth=*/64, /*isUnsigned=*/false);
+    ApInt = static_cast<std::int64_t>(Value);
+    return ApInt;
----------------
I believe you can do `return APSInt::get(static_cast<int64_t>(Value));` instead -- it should do the same thing.


================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:36
+
+static bool InsideMacroDefinition(const MatchFinder::MatchResult &Result,
+                                  SourceRange Range) {
----------------
This function name doesn't seem to relate to the behavior of the function? Rather than try to pin it down like that, perhaps rename to "CanRangeBeSafelyReplaced()` and add a comment as to why this is the correct approach for answering the question.


================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:54
+                                hasSourceExpression(expr().bind("cast_arg"))),
+                            cStyleCastExpr(
+                                hasDestinationType(realFloatingPointType()),
----------------
What about function-style casts? e.g. `float(1)`


================
Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:74
+
+  // Check for static casts to `float`.
+  if (const auto *MaybeCastArg = Result.Nodes.getNodeAs<Expr>("cast_arg")) {
----------------
Comment is now out of date, this checks for a few kinds of casts.


https://reviews.llvm.org/D53339