[clang-tidy] Fixed checker for abseil to work in C++17 mode

[jvikstrom]

Summary

Fixes the checker for abseil to make tests pass in C++17 mode

[hokein]

Thanks for investigating these, nice work!

Some initial comments:

• since your patch fixes four different check, I'd suggest to separate it (one patch per check)
• there are some non-functional changes (code format), I'd avoid them in this patch (we could address them in a separate patch)
• it would be nice if you could briefly describe the AST difference between C++11 and C++17 (how the fix work) in the patch description or comments

clang-tools-extra/test/clang-tidy/abseil-duration-unnecessary-conversion.cpp

1

I believe the tests are also passed for C++2a? use C++11-or-later will do the trick.

[gribozavr]

clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp

86

hasDescendant will find any child, no matter how deeply nested. I think in this case we only want the expression itself to be equal to ByAnyChar call. Shouldn't this code use the elidable matcher you're adding in another file?

121

Unrelated edit?

clang-tools-extra/clang-tidy/abseil/TimeSubtractionCheck.cpp

34

Ditto, hasAncestor can go too far up the tree.

[jvikstrom]

Resubmitted as 4 different CLs

clang-tools-extra/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp

 //===--- DurationUnnecessaryConversionCheck.cpp - clang-tidy
 //-----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "DurationUnnecessaryConversionCheck.h"
 #include "DurationRewriter.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/FixIt.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
+ast_matchers::internal::Matcher<Expr>
+elidable(ast_matchers::internal::Matcher<Expr> InnerMatcher) {
+  return anyOf(cxxConstructExpr(hasArgument(0, InnerMatcher)), InnerMatcher);
+}
+
 void DurationUnnecessaryConversionCheck::registerMatchers(MatchFinder *Finder) {
   for (const auto &Scale : {"Hours", "Minutes", "Seconds", "Milliseconds",
                             "Microseconds", "Nanoseconds"}) {
     std::string DurationFactory = (llvm::Twine("::absl::") + Scale).str();
     std::string FloatConversion =
         (llvm::Twine("::absl::ToDouble") + Scale).str();
     std::string IntegerConversion =
         (llvm::Twine("::absl::ToInt64") + Scale).str();
 
     // Matcher which matches the current scale's factory with a `1` argument,
     // e.g. `absl::Seconds(1)`.
-    auto factory_matcher = cxxConstructExpr(hasArgument(
-        0,
+
+    auto factory_matcher = elidable(
         callExpr(callee(functionDecl(hasName(DurationFactory))),
-                 hasArgument(0, ignoringImpCasts(integerLiteral(equals(1)))))));
+                 hasArgument(0, ignoringImpCasts(integerLiteral(equals(1))))));
 
     // Matcher which matches either inverse function and binds its argument,
     // e.g. `absl::ToDoubleSeconds(dur)`.
     auto inverse_function_matcher = callExpr(
         callee(functionDecl(hasAnyName(FloatConversion, IntegerConversion))),
         hasArgument(0, expr().bind("arg")));
 
     // Matcher which matches a duration divided by the factory_matcher above,

clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp

 //===--- FasterStrsplitDelimiterCheck.cpp - clang-tidy---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "FasterStrsplitDelimiterCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 namespace {
llvm::Optional<std::string> makeCharacterLiteral(const StringLiteral *Literal) {
   }
 
   // Special case: If the string contains a single quote, we just need to return
   // a character of the single quote. This is a special case because we need to
   // escape it in the character literal.
   if (Result == R"("'")")
     return std::string(R"('\'')");
 
-  assert(Result.size() == 3 || (Result.size() == 4 && Result.substr(0, 2) == "\"\\"));
+  assert(Result.size() == 3 ||
+         (Result.size() == 4 && Result.substr(0, 2) == "\"\\"));
 
   // Now replace the " with '.
   auto Pos = Result.find_first_of('"');
   if (Pos == Result.npos)
     return llvm::None;
   Result[Pos] = '\'';
   Pos = Result.find_last_of('"');
   if (Pos == Result.npos)
void FasterStrsplitDelimiterCheck::registerMatchers(MatchFinder *Finder) {
 
   // Binds to one character string literals.
   const auto SingleChar =
       expr(ignoringParenCasts(stringLiteral(lengthIsOne()).bind("Literal")));
 
   // Binds to a string_view (either absl or std) that was passed by value and
   // contructed from string literal.
   auto StringViewArg =
-      copyConstructExprWithArg("::absl::string_view", SingleChar);
+      anyOf(copyConstructExprWithArg("::absl::string_view", SingleChar),
+            constructExprWithArg("::absl::string_view", SingleChar));
 
   auto ByAnyCharArg =
-      expr(copyConstructExprWithArg("::absl::ByAnyChar", StringViewArg))
-          .bind("ByAnyChar");
+      anyOf(expr(copyConstructExprWithArg("::absl::ByAnyChar", StringViewArg))
+                .bind("ByAnyChar"),
+            expr(hasDescendant(cxxConstructExpr(
+                     hasType(recordDecl(hasName("::absl::ByAnyChar"))),
+                     hasArgument(0, ignoringParenCasts(StringViewArg)))))
+                .bind("ByAnyChar"));
 
   // Find uses of absl::StrSplit(..., "x") and absl::StrSplit(...,
   // absl::ByAnyChar("x")) to transform them into absl::StrSplit(..., 'x').
   Finder->addMatcher(callExpr(callee(functionDecl(hasName("::absl::StrSplit"))),
                               hasArgument(1, anyOf(ByAnyCharArg, SingleChar)),
                               unless(isInTemplateInstantiation()))
                          .bind("StrSplit"),
                      this);
void FasterStrsplitDelimiterCheck::check(
   const auto *Literal = Result.Nodes.getNodeAs<StringLiteral>("Literal");
 
   if (Literal->getBeginLoc().isMacroID() || Literal->getEndLoc().isMacroID())
     return;
 
   llvm::Optional<std::string> Replacement = makeCharacterLiteral(Literal);
   if (!Replacement)
     return;
+
   SourceRange Range = Literal->getSourceRange();
 
   if (const auto *ByAnyChar = Result.Nodes.getNodeAs<Expr>("ByAnyChar"))
     Range = ByAnyChar->getSourceRange();
 
   diag(
       Literal->getBeginLoc(),
       "%select{absl::StrSplit()|absl::MaxSplits()}0 called with a string "
       "literal "

clang-tools-extra/clang-tidy/abseil/TimeSubtractionCheck.cpp

 //===--- TimeSubtractionCheck.cpp - clang-tidy ----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "TimeSubtractionCheck.h"
 #include "DurationRewriter.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/FixIt.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 // Returns `true` if `Range` is inside a macro definition.
 static bool InsideMacroDefinition(const MatchFinder::MatchResult &Result,
                                   SourceRange Range) {
   return !clang::Lexer::makeFileCharRange(
               clang::CharSourceRange::getCharRange(Range),
               *Result.SourceManager, Result.Context->getLangOpts())
               .isValid();
 }
 
 static bool isConstructorAssignment(const MatchFinder::MatchResult &Result,
                                     const Expr *Node) {
-  return selectFirst<const Expr>(
-             "e", match(expr(hasParent(materializeTemporaryExpr(hasParent(
-                                 cxxConstructExpr(hasParent(exprWithCleanups(
-                                     hasParent(varDecl()))))))))
-                            .bind("e"),
+  return selectFirst<const Expr>("e",
+                                 match(expr(hasAncestor(varDecl())).bind("e"),
                                        *Node, *Result.Context)) != nullptr;
 }
 
 static bool isArgument(const MatchFinder::MatchResult &Result,
                        const Expr *Node) {
   return selectFirst<const Expr>(
-             "e",
-             match(expr(hasParent(
-                            materializeTemporaryExpr(hasParent(cxxConstructExpr(
-                                hasParent(callExpr()),
-                                unless(hasParent(cxxOperatorCallExpr())))))))
+             "e", match(expr(hasAncestor(callExpr()),
+                             unless(hasAncestor(cxxOperatorCallExpr())))
                             .bind("e"),
                         *Node, *Result.Context)) != nullptr;
 }
 
 static bool isReturn(const MatchFinder::MatchResult &Result, const Expr *Node) {
   return selectFirst<const Expr>(
-             "e", match(expr(hasParent(materializeTemporaryExpr(hasParent(
-                                 cxxConstructExpr(hasParent(exprWithCleanups(
-                                     hasParent(returnStmt()))))))))
-                            .bind("e"),
-                        *Node, *Result.Context)) != nullptr;
+             "e", match(expr(hasAncestor(returnStmt())).bind("e"), *Node,
+                        *Result.Context)) != nullptr;
 }
 
 static bool parensRequired(const MatchFinder::MatchResult &Result,
                            const Expr *Node) {
   // TODO: Figure out any more contexts in which we can omit the surrounding
   // parentheses.
   return !(isConstructorAssignment(Result, Node) || isArgument(Result, Node) ||
            isReturn(Result, Node));
for (auto ScaleName :
     // subtracting a 'Time' from a 'Duration' is not defined, in these cases,
     // we always know the first operand is a 'Time' if the second is a 'Time'.
     auto OperandMatcher =
         binaryOperator(hasOperatorName("-"), hasRHS(TimeInverseMatcher))
             .bind("binop");
     Finder->addMatcher(OperandMatcher, this);
   }
 }
-
 void TimeSubtractionCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binop");
   std::string InverseName =
       Result.Nodes.getNodeAs<FunctionDecl>("func_decl")->getNameAsString();
   if (InsideMacroDefinition(Result, BinOp->getSourceRange()))
     return;
 
   llvm::Optional<DurationScale> Scale = getScaleForTimeInverse(InverseName);

clang-tools-extra/clang-tidy/abseil/UpgradeDurationConversionsCheck.h

 #include "../ClangTidy.h"
 
 #include <unordered_set>
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
-/// Finds deprecated uses of `absl::Duration` arithmetic operators and factories.
+/// Finds deprecated uses of `absl::Duration` arithmetic operators and
+/// factories.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/abseil-upgrade-duration-conversions.html
 class UpgradeDurationConversionsCheck : public ClangTidyCheck {
 public:
   UpgradeDurationConversionsCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;

clang-tools-extra/clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp

 //===--- UpgradeDurationConversionsCheck.cpp - clang-tidy -----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "UpgradeDurationConversionsCheck.h"
 #include "DurationRewriter.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/Specifiers.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 void UpgradeDurationConversionsCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
 
   // For the arithmetic calls, we match only the uses of the templated operators
   // where the template parameter is not a built-in type. This means the
   // instantiation makes use of an available user defined conversion to
   // `int64_t`.
   //
   // The implementation of these templates will be updated to fail SFINAE for
   // non-integral types. We match them to suggest an explicit cast.
 
   // Match expressions like `a *= b` and `a /= b` where `a` has type
   // `absl::Duration` and `b` is not of a built-in type.
   Finder->addMatcher(
       cxxOperatorCallExpr(
           argumentCountIs(2),
-          hasArgument(
-              0, expr(hasType(cxxRecordDecl(hasName("::absl::Duration"))))),
+          hasArgument(0,
+                      expr(hasType(cxxRecordDecl(hasName("::absl::Duration"))))
+                          .bind("arg0")),
           hasArgument(1, expr().bind("arg")),
           callee(functionDecl(
               hasParent(functionTemplateDecl()),
               unless(hasTemplateArgument(0, refersToType(builtinType()))),
-              hasAnyName("operator*=", "operator/=")))),
+              hasAnyName("operator*=", "operator/="))))
+          .bind("operator"),
       this);
 
   // Match expressions like `a.operator*=(b)` and `a.operator/=(b)` where `a`
   // has type `absl::Duration` and `b` is not of a built-in type.
   Finder->addMatcher(
       cxxMemberCallExpr(
           callee(cxxMethodDecl(
               ofClass(cxxRecordDecl(hasName("::absl::Duration"))),
               hasParent(functionTemplateDecl()),
               unless(hasTemplateArgument(0, refersToType(builtinType()))),
               hasAnyName("operator*=", "operator/="))),
-          argumentCountIs(1), hasArgument(0, expr().bind("arg"))),
+          argumentCountIs(1), hasArgument(0, expr().bind("arg")))
+          .bind("operator"),
       this);
 
   // Match expressions like `a * b`, `a / b`, `operator*(a, b)`, and
   // `operator/(a, b)` where `a` has type `absl::Duration` and `b` is not of a
   // built-in type.
   Finder->addMatcher(
       callExpr(callee(functionDecl(
                    hasParent(functionTemplateDecl()),
                    unless(hasTemplateArgument(0, refersToType(builtinType()))),
                    hasAnyName("::absl::operator*", "::absl::operator/"))),
                argumentCountIs(2),
                hasArgument(0, expr(hasType(
                                   cxxRecordDecl(hasName("::absl::Duration"))))),
-               hasArgument(1, expr().bind("arg"))),
+               hasArgument(1, expr().bind("arg")))
+          .bind("operator"),
       this);
 
   // Match expressions like `a * b` and `operator*(a, b)` where `a` is not of a
   // built-in type and `b` has type `absl::Duration`.
   Finder->addMatcher(
       callExpr(callee(functionDecl(
                    hasParent(functionTemplateDecl()),
                    unless(hasTemplateArgument(0, refersToType(builtinType()))),
                    hasName("::absl::operator*"))),
                argumentCountIs(2), hasArgument(0, expr().bind("arg")),
-               hasArgument(1, expr(hasType(cxxRecordDecl(
-                                  hasName("::absl::Duration")))))),
+               hasArgument(1, expr(hasType(
+                                  cxxRecordDecl(hasName("::absl::Duration"))))))
+          .bind("operator"),
       this);
 
   // For the factory functions, we match only the non-templated overloads that
   // take an `int64_t` parameter. Within these calls, we care about implicit
   // casts through a user defined conversion to `int64_t`.
   //
   // The factory functions will be updated to be templated and SFINAE on whether
   // the template parameter is an integral type. This complements the already
   // existing templated overloads that only accept floating point types.
 
   // Match calls like:
   //   `absl::Nanoseconds(x)`
   //   `absl::Microseconds(x)`
   //   `absl::Milliseconds(x)`
   //   `absl::Seconds(x)`
   //   `absl::Minutes(x)`
   //   `absl::Hours(x)`
   // where `x` is not of a built-in type.
   Finder->addMatcher(
       implicitCastExpr(
           anyOf(hasCastKind(CK_UserDefinedConversion),
                 has(implicitCastExpr(hasCastKind(CK_UserDefinedConversion)))),
           hasParent(callExpr(
               callee(functionDecl(DurationFactoryFunction(),
                                   unless(hasParent(functionTemplateDecl())))),
-              hasArgument(0, expr().bind("arg"))))),
+              hasArgument(0, expr().bind("arg")))))
+          .bind("operator"),
       this);
 }
 
 void UpgradeDurationConversionsCheck::check(
     const MatchFinder::MatchResult &Result) {
   const llvm::StringRef Message =
       "implicit conversion to 'int64_t' is deprecated in this context; use an "
       "explicit cast instead";
 
   const auto *ArgExpr = Result.Nodes.getNodeAs<Expr>("arg");
-  SourceLocation Loc = ArgExpr->getBeginLoc();
 
-  if (!match(isInTemplateInstantiation(), *ArgExpr, *Result.Context).empty()) {
+  const auto *OperatorExpr = Result.Nodes.getNodeAs<Expr>("operator");
+
+  SourceLocation Loc = ArgExpr->getBeginLoc();
+  if (!match(isInTemplateInstantiation(), *OperatorExpr, *Result.Context)
+           .empty()) {
     if (MatchedTemplateLocations.count(Loc.getRawEncoding()) == 0) {
       // For each location matched in a template instantiation, we check if the
       // location can also be found in `MatchedTemplateLocations`. If it is not
       // found, that means the expression did not create a match without the
       // instantiation and depends on template parameters. A manual fix is
       // probably required so we provide only a warning.
       diag(Loc, Message);
     }
+
     return;
   }
 
   // We gather source locations from template matches not in template
   // instantiations for future matches.
   internal::Matcher<Stmt> IsInsideTemplate =
       hasAncestor(decl(anyOf(classTemplateDecl(), functionTemplateDecl())));
   if (!match(IsInsideTemplate, *ArgExpr, *Result.Context).empty())

clang-tools-extra/test/clang-tidy/abseil-duration-unnecessary-conversion.cpp

-// RUN: %check_clang_tidy -std=c++11,c++14 %s abseil-duration-unnecessary-conversion %t -- -- -I %S/Inputs
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy -std=c++11,c++14,c++17 %s abseil-duration-unnecessary-conversion %t -- -- -I %S/Inputs
 
 #include "absl/time/time.h"
 
 void f() {
   absl::Duration d1, d2;
 
   // Floating point
   d2 = absl::Hours(absl::ToDoubleHours(d1));

clang-tools-extra/test/clang-tidy/abseil-faster-strsplit-delimiter.cpp

-// RUN: %check_clang_tidy -std=c++11,c++14 %s abseil-faster-strsplit-delimiter %t
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy -std=c++11,c++14,c++17 %s abseil-faster-strsplit-delimiter %t
 
 namespace absl {
 
 class string_view {
   public:
     string_view();
     string_view(const char *);
 };

clang-tools-extra/test/clang-tidy/abseil-time-subtraction.cpp

-// RUN: %check_clang_tidy -std=c++11,c++14 %s abseil-time-subtraction %t -- -- -I %S/Inputs
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy -std=c++11,c++14,c++17 %s abseil-time-subtraction %t -- -- -I %S/Inputs
 
 #include "absl/time/time.h"
 
 void g(absl::Duration d);
 
 void f() {
   absl::Time t;
   int x, y;

clang-tools-extra/test/clang-tidy/abseil-upgrade-duration-conversions.cpp

-// RUN: %check_clang_tidy -std=c++11,c++14 %s abseil-upgrade-duration-conversions %t -- -- -I%S/Inputs
-// FIXME: Fix the checker to work in C++17 mode.
+// RUN: %check_clang_tidy -std=c++11,c++14,c++17 %s abseil-upgrade-duration-conversions %t -- -- -I%S/Inputs
 
 using int64_t = long long;
 
 #include "absl/time/time.h"
 
 template <typename T> struct ConvertibleTo {
   operator T() const;
 };