[PATCH] D56532: [clang-tidy] Add the abseil-duration-conversion-cast check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:47-48
   // if nothing needs to be done.
-  if (!IsValidMacro(Result, Binop->getLHS()) ||
-      !IsValidMacro(Result, Binop->getRHS()))
+  if (!isNotInMacro(Result, Binop->getLHS()) ||
+      !isNotInMacro(Result, Binop->getRHS()))
     return;
----------------
I think this change (and the related removal) should be landed separately as a NFC commit once this one lands, as it's logically separate from the new check.


================
Comment at: clang-tidy/abseil/DurationConversionCastCheck.cpp:46
+  const auto *Arg = Result.Nodes.getNodeAs<Expr>("arg");
+  llvm::StringRef ConversionFuncName = FuncDecl->getName();
+
----------------
You can drop the `llvm::` qualifiers here and elsewhere.


================
Comment at: clang-tidy/abseil/DurationConversionCastCheck.cpp:52
+
+  // Casting a double to an integer
+  if (MatchedCast->getTypeAsWritten()->isIntegerType() &&
----------------
Add a full stop to the end of the comment (same below).


================
Comment at: clang-tidy/abseil/DurationConversionCastCheck.cpp:57
+
+    diag(MatchedCast->getBeginLoc(), "convert duration directly to integer")
+        << FixItHint::CreateReplacement(
----------------
This diagnostic message does not tell the user what they did wrong with their code. How about `duration should be converted directly to integer|floating-point rather than through a type cast` or something along those lines?


================
Comment at: test/clang-tidy/abseil-duration-conversion-cast.cpp:10
+
+  i = static_cast<int>(absl::ToDoubleHours(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to integer [abseil-duration-conversion-cast]
----------------
Can you add an example like:
```
typedef int FancyInt;

FancyInt j = static_cast<FancyInt>(absl::ToDoubleHours(d1));
```
to make sure the cast is looking through types as expected? Please add a similar test for floating-point casts.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56532/new/

https://reviews.llvm.org/D56532