[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Hi astrelni,

my 2cents.
Please upload the patch will full context (i believe `diff -U 99999`, but check the man pages if that doesn't work :D)



================
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:32
+
+  // a *= b; a /= b;
+  Finder->addMatcher(
----------------
Please add types in your examples, to make clear what actually happens.


================
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:118
+AST_MATCHER_P(Expr, hasSourceRange, SourceRange, Range) {
+  return Node.getSourceRange() == Range;
+}
----------------
What happens on invalid ranges? Are they considered equal, or is it forbidden to pass them in?


================
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:123
+  auto HasMatchingDependentDescendant = hasDescendant(
+      expr(hasSourceRange(Node.getSourceRange()), isInstantiationDependent()));
+  return expr(anyOf(hasAncestor(
----------------
Please add a test-case where the `Node.getSourceRange()` is a macro, if not already existing. I believe that is currently missing.


================
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:134
+void UpgradeDurationConversionsCheck::check(
+    const ast_matchers::MatchFinder::MatchResult &Result) {
+  const auto *ArgExpr = Result.Nodes.getNodeAs<Expr>("arg");
----------------
`ast_matchers::` is not necessary, as there is a `using ...` at the top of the file.


================
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:158
+               *Result.Context)
+             .empty()) {
+      diag(ArgExpr->getBeginLoc(), Message);
----------------
You could ellide these braces, but I feel that this matching code be merged into one matcher with `equalsBoundNode()` (see ASTMatcher reference).


================
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:164
+
+  FixItHint Fixes[] = {FixItHint::CreateInsertion(SourceRange.getBegin(),
+                                                  "static_cast<int64_t>("),
----------------
my personal preference would be removing the builtin-array and add two `<< FixitHint` call to the `diag`.


================
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.h:19
+
+/// Finds deprecated uses of absl::Duration arithmetic operators and factories.
+///
----------------
please mark code construct with ' in comments.


================
Comment at: docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst:43
+behavior due to types implicitly convertible to a floating point type.
+
----------------
please remove the last empty line


================
Comment at: docs/clang-tidy/checks/list.rst:13
    abseil-redundant-strcat-calls
+   abseil-str-cat-append
    abseil-string-find-startswith
----------------
spurious changes in this file.


================
Comment at: test/clang-tidy/abseil-upgrade-duration-conversions.cpp:142
+
+template <typename T> void templateForOpsSpecialization(T) {}
+template <>
----------------
what about non-type template parameters? Do they need consideration for the check as well?
If i am not mistaken floats are allowed in newwer standards as well.


https://reviews.llvm.org/D53830