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

[Alex Strelnikov via Phabricator via cfe-commits]

astrelni marked an inline comment as done.
astrelni added inline comments.


================
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:118
+AST_MATCHER_P(Expr, hasSourceRange, SourceRange, Range) {
+  return Node.getSourceRange() == Range;
+}
----------------
JonasToth wrote:
> What happens on invalid ranges? Are they considered equal, or is it forbidden to pass them in?
Good point. I think this is a non-issue with the way the code is set up now, but better make it explicitly work :). We only care about valid source ranges. I've updated the name of the matcher and added a check here for the Node's source range and another in the matcher below to return false early.


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


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

Sorry I had a look at `equalsBoundNode()`, but couldn't see exactly what you meant. Could you please elaborate about the merging?


================
Comment at: test/clang-tidy/abseil-upgrade-duration-conversions.cpp:142
+
+template <typename T> void templateForOpsSpecialization(T) {}
+template <>
----------------
JonasToth wrote:
> 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.
IIUC non-type template parameters should be no different for this check. This particular case is to make sure explicit specializations are treated differently from instantiations and get fix-it hints. 


https://reviews.llvm.org/D53830