[ASTMatchers] Adds `CXXMemberCallExpr` matcher `invokedAtType`.

[ymandel]

Summary

This matcher is a variant of thisPointerType, like on is a variant of onImplicitObjectArgument.

We also add a test for the matcher and update reference HTML accordingly.

[lebedev.ri]

Please always subscribe lists.
The reviews that happen without lists are null and void.

[alexfh]

clang/include/clang/ASTMatchers/ASTMatchers.h

3300 ↗

(On Diff #182263)

The name of the matcher doesn't tell me much. I had to carefully read the documentation to understand what is it about. I don't have a name that would raise no questions and wouldn't be too verbose at the same time, but a bit of verbosity wouldn't hurt I guess. How about objectTypeAsWritten?

3301 ↗

(On Diff #182263)

nit: clang:: and clang::ast_matchers:: are unnecessary. Same below

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

3300 ↗

(On Diff #182263)

Yeah, I think this would be a better name. Also, having some examples that demonstrate where this behavior differs from thisPointerType would be helpful.

[ymandel]

clang/include/clang/ASTMatchers/ASTMatchers.h

3300 ↗

(On Diff #182263)

Agreed that it needs a new name, but I'm having trouble finding one I'm satisfied with. Here's the full description: "the type of the written implicit object argument". I base this phrasing on the class CXXMemberCallExpr's terminology. In x.f(5), x is the implicit object argument, whether or not it is also implicitly surrounded by a cast. That is, "implicit" has two different meanings in this context.

So, with that, how about writtenObjectType? It's close to objectTypeAsWritten but I'm hoping it makes more clear that the "written" part is the object not the type.

[ymandel]

Remove unneeded qualifiers and clarify constrast with thisPointerType.

[ymandel]

clang/include/clang/ASTMatchers/ASTMatchers.h

3300 ↗

(On Diff #182263)

I've contrasted the behavior with thisPointerType in both of the examples. Do you think this helps or do you want something more explicit?

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

3300 ↗

(On Diff #182263)

Here's a totally different direction: onOrPointsToType()

cxxMemberCallExpr(onOrPointsToType(hasDeclaration(cxxRecordDecl(hasName("Y")))))

3300 ↗

(On Diff #182263)

I think more explicit would be better. e.g.,

cxxMemberCallExpr(invokedAtType(hasDeclaration(cxxRecordDecl(hasName("X")))))
matches 'x.m()' and 'p->m()'.
cxxMemberCallExpr(on(thisPointerType(hasDeclaration(cxxRecordDecl(hasName("X"))))))
matches nothing because the type of 'this' is 'Y' in both cases.

[ymandel]

Contrast to thisPointerType with explicit examples.

[ymandel]

clang/include/clang/ASTMatchers/ASTMatchers.h

3300 ↗

(On Diff #182263)

But, what about even simpler: onType? I think this parallels the intuition of the name thisPointerType. onType(T) should match x.f and x->f, where x is type T.

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

3300 ↗

(On Diff #182263)

You've pointed out why I don't think onType works -- it doesn't match on type T -- it matches on type T, or a pointer/reference to type T, which is pretty different. Someone reading the matcher may expect an exact type match and insert a pointerType() or something there thinking they need to do that to match a call through a pointer.

@alexfh, opinions?

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

473 ↗

(On Diff #183591)

The test name is using the old name for the matcher.

474 ↗

(On Diff #183591)

Same here as well.

[ymandel]

clang/include/clang/ASTMatchers/ASTMatchers.h

3300 ↗

(On Diff #182263)

True. I should have explained more.

1. Ultimately, I think that none of these names really make sense on their own and the user will need some familiarity with the documentation. I spent quite a while trying to come up with better names and didn't find anything compelling. I think that onType benefits from not carrying much information -- reducing the likelihood of misunderstanding it (they'll have to read the documentation) while paralleling the meaning of the matcher on and the behavior of thisPointerType (which also allows either the type or the pointer to that type).

2. My particular concern with onOrPointsToType is that it sounds like the "or" applies to the on but it really means "on (type or points to type)".

[alexfh]

clang/include/clang/ASTMatchers/ASTMatchers.h

3300 ↗

(On Diff #182263)

So far, my observations are:

1. three engineers quite familiar with the topic can't come up with a name that would explain the concept behind this matcher
2. anyone reading that name would have to look up the documentation
3. the implementation of the matcher is straightforward and even shorter than the documentation

Should we give up and let users just type on(anyOf(hasType(Q), hasType(pointsTo(Q))))?

If we want a bit more brevity here, maybe introduce a hasTypeOrPointsToType matcher (any bikeshed color will do ;) to shorten the expression above?

[ymandel]

clang/include/clang/ASTMatchers/ASTMatchers.h

3300 ↗

(On Diff #182263)

Yes to both suggestions (dropping this one and adding hasTypeOrPointsToType). It seems a rather obvious conclusion now that you've said it. :)

Personally, I'd go with hasOrPointsToType, but agreed that its just bike shedding. Aaron?

I'll drop this diff and create a new one for the new matcher.

[aaron.ballman]

clang/include/clang/ASTMatchers/ASTMatchers.h

3300 ↗

(On Diff #182263)

Personally, I'd go with hasOrPointsToType, but agreed that its just bike shedding. Aaron?

I think hasOrPointsToType is sufficiently clear within this context, but it makes me wonder if users are going to then need hasOrPointsToOrReferencesType() for other situations?

I am kind of leaning towards just letting users spell the matcher out long-hand as on(anyOf(hasType(Q), hasType(pointsTo(Q))))

[ymandel]

clang/include/clang/ASTMatchers/ASTMatchers.h

3300 ↗

(On Diff #182263)

Sure. I'll drop this revision and we can revisit in the future if we see sufficient demand.

[ymandel]

This comment was removed by ymandel.

clang/include/clang/Tooling/FixIt.h

 //    tooling::fixit::createReplacement(*B, *A);
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLING_FIXIT_H
 #define LLVM_CLANG_TOOLING_FIXIT_H
 
 #include "clang/AST/ASTContext.h"
+#include "clang/Basic/TokenKinds.h"
 
 namespace clang {
 namespace tooling {
 namespace fixit {
 
 namespace internal {
-StringRef getText(SourceRange Range, const ASTContext &Context);
+StringRef getText(CharSourceRange Range, const ASTContext &Context);
 
-/// Returns the SourceRange of a SourceRange. This identity function is
-///        used by the following template abstractions.
-inline SourceRange getSourceRange(const SourceRange &Range) { return Range; }
+/// Returns the token CharSourceRange corresponding to \p Range.
+inline CharSourceRange getSourceRange(const SourceRange &Range) {
+  return CharSourceRange::getTokenRange(Range);
+}
 
-/// Returns the SourceRange of the token at Location \p Loc.
-inline SourceRange getSourceRange(const SourceLocation &Loc) {
-  return SourceRange(Loc);
+/// Returns the CharSourceRange of the token at Location \p Loc.
+inline CharSourceRange getSourceRange(const SourceLocation &Loc) {
+  return CharSourceRange::getTokenRange(Loc, Loc);
 }
 
-/// Returns the SourceRange of an given Node. \p Node is typically a
+/// Returns the CharSourceRange of an given Node. \p Node is typically a
 ///        'Stmt', 'Expr' or a 'Decl'.
-template <typename T> SourceRange getSourceRange(const T &Node) {
-  return Node.getSourceRange();
+template <typename T> CharSourceRange getSourceRange(const T &Node) {
+  return CharSourceRange::getTokenRange(Node.getSourceRange());
 }
+
+/// Extends \p Range to include the token \p Next, if it immediately follows the
+/// end of the range. Otherwise, returns \p Range unchanged.
+CharSourceRange maybeExtendRange(CharSourceRange Range, tok::TokenKind Next,
+                                 ASTContext &Context);
 } // end namespace internal
 
-// Returns a textual representation of \p Node.
+/// Returns a textual representation of \p Node.
 template <typename T>
 StringRef getText(const T &Node, const ASTContext &Context) {
   return internal::getText(internal::getSourceRange(Node), Context);
 }
 
+/// Returns the source range spanning the node, extended to include \p Next, if
+/// it immediately follows \p Node. Otherwise, returns the normal range of \p
+/// Node.  See comments on `getExtendedText()` for examples.
+template <typename T>
+CharSourceRange getExtendedRange(const T &Node, tok::TokenKind Next,
+                                 ASTContext &Context) {
+  return internal::maybeExtendRange(internal::getSourceRange(Node), Next,
+                                    Context);
+}
+
+/// Returns the source text of the node, extended to include \p Next, if it
+/// immediately follows the node. Otherwise, returns the text of just \p Node.
+///
+/// For example, given statements S1 and S2 below:
+/// \code
+///   {
+///     // S1:
+///     if (!x) return foo();
+///     // S2:
+///     if (!x) { return 3; }
+//    }
+/// \endcode
+/// then
+/// \code
+///   getText(S1, Context) = "if (!x) return foo()"
+///   getExtendedText(S1, tok::TokenKind::semi, Context)
+///     = "if (!x) return foo();"
+///   getExtendedText(*S1.getThen(), tok::TokenKind::semi, Context)
+///     = "return foo();"
+///   getExtendedText(*S2.getThen(), tok::TokenKind::semi, Context)
+///     = getText(S2, Context) = "{ return 3; }"
+/// \endcode
+template <typename T>
+StringRef getExtendedText(const T &Node, tok::TokenKind Next,
+                          ASTContext &Context) {
+  return internal::getText(getExtendedRange(Node, Next, Context), Context);
+}
+
 // Returns a FixItHint to remove \p Node.
 // TODO: Add support for related syntactical elements (i.e. comments, ...).
 template <typename T> FixItHint createRemoval(const T &Node) {
   return FixItHint::CreateRemoval(internal::getSourceRange(Node));
 }
 
 // Returns a FixItHint to replace \p Destination by \p Source.
 template <typename D, typename S>

clang/lib/Tooling/FixIt.cpp

 #include "clang/Tooling/FixIt.h"
 #include "clang/Lex/Lexer.h"
 
 namespace clang {
 namespace tooling {
 namespace fixit {
 
 namespace internal {
-StringRef getText(SourceRange Range, const ASTContext &Context) {
-  return Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
-                              Context.getSourceManager(),
+StringRef getText(CharSourceRange Range, const ASTContext &Context) {
+  return Lexer::getSourceText(Range, Context.getSourceManager(),
                               Context.getLangOpts());
 }
-} // end namespace internal
+
+CharSourceRange maybeExtendRange(CharSourceRange Range, tok::TokenKind Next,
+                                 ASTContext &Context) {
+  Optional<Token> Tok = Lexer::findNextToken(
+      Range.getEnd(), Context.getSourceManager(), Context.getLangOpts());
+  if (!Tok || !Tok->is(Next))
+    return Range;
+  return CharSourceRange::getTokenRange(Range.getBegin(), Tok->getLocation());
+}
+} // namespace internal
 
 } // end namespace fixit
 } // end namespace tooling
 } // end namespace clang

clang/unittests/Tooling/FixItTest.cpp

 //===- unittest/Tooling/FixitTest.cpp ------------------------------------===//
 //
 // 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 "TestVisitor.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Tooling/FixIt.h"
 
 using namespace clang;
 
 using tooling::fixit::getText;
+using tooling::fixit::getExtendedText;
 using tooling::fixit::createRemoval;
 using tooling::fixit::createReplacement;
 
 namespace {
 
 struct CallsVisitor : TestVisitor<CallsVisitor> {
   bool VisitCallExpr(CallExpr *Expr) {
     OnCall(Expr, Context);
Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
     Expr *P1 = CE->getArg(1);
     EXPECT_EQ("x", getText(*P0, *Context));
     EXPECT_EQ("y", getText(*P1, *Context));
   };
   Visitor.runOver("#define FOO(x, y) (void)x; (void)y; foo(x, y);\n"
                   "void foo(int x, int y) { FOO(x,y) }");
 }
 
+TEST(FixItTest, getExtendedText) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    EXPECT_EQ("foo(x, y);",
+              getExtendedText(*CE, tok::TokenKind::semi, *Context));
+
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    EXPECT_EQ("x", getExtendedText(*P0, tok::TokenKind::semi, *Context));
+    EXPECT_EQ("x,", getExtendedText(*P0, tok::TokenKind::comma, *Context));
+    EXPECT_EQ("y", getExtendedText(*P1, tok::TokenKind::semi, *Context));
+  };
+  Visitor.runOver("void foo(int x, int y) { foo(x, y); }");
+  Visitor.runOver("void foo(int x, int y) { if (true) foo(x, y); }");
+  Visitor.runOver("int foo(int x, int y) { if (true) return 3 + foo(x, y); }");
+  Visitor.runOver("void foo(int x, int y) { for (foo(x, y);;) ++x; }");
+  Visitor.runOver(
+      "bool foo(int x, int y) { for (;foo(x, y);) x = 1; return true; }");
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    EXPECT_EQ("foo()", getExtendedText(*CE, tok::TokenKind::semi, *Context));
+  };
+  Visitor.runOver("bool foo() { if (foo()) return true; return false; }");
+  Visitor.runOver("void foo() { int x; for (;; foo()) ++x; }");
+  Visitor.runOver("int foo() { return foo() + 3; }");
+}
+
 TEST(FixItTest, createRemoval) {
   CallsVisitor Visitor;
 
   Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
     FixItHint Hint = createRemoval(*CE);
     EXPECT_EQ("foo(x, y)", getText(Hint.RemoveRange.getAsRange(), *Context));
     EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
     EXPECT_TRUE(Hint.CodeToInsert.empty());