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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
