[libTooling] Add stencil combinators for nodes that may be pointers or values.

[ymandel]

Summary

Adds combinators maybeDeref and maybeAddressOf to provide a uniform way to handle
nodes which may be bound to either a pointer or a value (most often in the
context of member expressions). Such polymorphism is already supported by
access; these combinators extend it to more general uses.

[gribozavr2]

clang/include/clang/Tooling/Transformer/Stencil.h

101

Three slashes, please.

102

Need more explanation to cover the cases when the expression is a pointer to a pointer (do we deref once or twice?), or a smart pointer (do we take care of that at all?)

WDYT about calling it maybeDeref() or derefIfPointer()?

105

Three slashes, please.

107

Similarly, maybeAddressOf()?

[ymandel]

renamed combinators; reordered cases to pair x and maybeX.

[ymandel]

fix typo

[ymandel]

Thanks for the review!

clang/include/clang/Tooling/Transformer/Stencil.h

102

I went with maybe... Given that, I didn't elaborate on pointer-to-pointer case because it never claims to result in a value. WDYT?

also, I kind of like derefIfPointer better, but addressOfIfValue just seemed to clunky. So, went w/ maybe...

[gribozavr2]

clang/include/clang/Tooling/Transformer/Stencil.h

90

"If the expression \p ExprId has pointer type ..."

101

"If the expression \p ExprId is not a pointer, ..."

[gribozavr2]

clang/include/clang/Tooling/Transformer/Stencil.h

102

Given that, I didn't elaborate on pointer-to-pointer case because it never claims to result in a value. WDYT?

I think it is clear what happens with pointers to pointers given the current doc comment.

clang/include/clang/Tooling/Transformer/Stencil.h

 /// binary operation is always wrapped, while a variable reference is never
 /// wrapped.
 Stencil expression(llvm::StringRef Id);
 
 /// Constructs an idiomatic dereferencing of the expression bound to \p ExprId.
 /// \p ExprId is wrapped in parentheses, if needed.
 Stencil deref(llvm::StringRef ExprId);
 
+/// If \p ExprId is of pointer type, constructs an idiomatic dereferencing of
+/// the expression bound to \p ExprId, including wrapping it in parentheses, if
+/// needed. Otherwise, generates the original expression source.
+/// FIXME: Identify smart points as pointer types.
+Stencil maybeDeref(llvm::StringRef ExprId);
+
 /// Constructs an expression that idiomatically takes the address of the
 /// expression bound to \p ExprId. \p ExprId is wrapped in parentheses, if
 /// needed.
 Stencil addressOf(llvm::StringRef ExprId);
 
+/// If \p ExprId is not a pointer type, constructs an expression that
+/// idiomatically takes the address of the expression bound to \p ExprId,
+/// including wrapping \p ExprId in parentheses, if needed. Otherwise, generates
+/// the original expression source.
+/// FIXME: Identify smart points as pointer types.
+Stencil maybeAddressOf(llvm::StringRef ExprId);
+
 /// Constructs a `MemberExpr` that accesses the named member (\p Member) of the
 /// object bound to \p BaseId. The access is constructed idiomatically: if \p
 /// BaseId is bound to `e` and \p Member identifies member `m`, then returns
 /// `e->m`, when e is a pointer, `e2->m` when e = `*e2` and `e.m` otherwise.
 /// Additionally, `e` is wrapped in parentheses, if needed.
 Stencil access(llvm::StringRef BaseId, Stencil Member);
 inline Stencil access(llvm::StringRef BaseId, llvm::StringRef Member) {
   return access(BaseId, text(Member));

clang/lib/Tooling/Transformer/Stencil.cpp

struct DebugPrintNodeData {
   explicit DebugPrintNodeData(std::string S) : Id(std::move(S)) {}
   std::string Id;
 };
 
 // Operators that take a single node Id as an argument.
 enum class UnaryNodeOperator {
   Parens,
   Deref,
-  Address,
+  MaybeDeref,
+  AddressOf,
+  MaybeAddressOf,
 };
 
 // Generic container for stencil operations with a (single) node-id argument.
 struct UnaryOperationData {
   UnaryOperationData(UnaryNodeOperator Op, std::string Id)
       : Op(Op), Id(std::move(Id)) {}
   UnaryNodeOperator Op;
   std::string Id;
std::string toStringData(const UnaryOperationData &Data) {
   StringRef OpName;
   switch (Data.Op) {
   case UnaryNodeOperator::Parens:
     OpName = "expression";
     break;
   case UnaryNodeOperator::Deref:
     OpName = "deref";
     break;
-  case UnaryNodeOperator::Address:
+  case UnaryNodeOperator::MaybeDeref:
+    OpName = "maybeDeref";
+    break;
+  case UnaryNodeOperator::AddressOf:
     OpName = "addressOf";
     break;
+  case UnaryNodeOperator::MaybeAddressOf:
+    OpName = "maybeAddressOf";
+    break;
   }
   return (OpName + "(\"" + Data.Id + "\")").str();
 }
 
 std::string toStringData(const SelectorData &) { return "selection(...)"; }
 
 std::string toStringData(const AccessData &Data) {
   return (llvm::Twine("access(\"") + Data.BaseId + "\", " +
Error evalData(const UnaryOperationData &Data,
   llvm::Optional<std::string> Source;
   switch (Data.Op) {
   case UnaryNodeOperator::Parens:
     Source = tooling::buildParens(*E, *Match.Context);
     break;
   case UnaryNodeOperator::Deref:
     Source = tooling::buildDereference(*E, *Match.Context);
     break;
-  case UnaryNodeOperator::Address:
+  case UnaryNodeOperator::MaybeDeref:
+    if (!E->getType()->isAnyPointerType()) {
+      *Result += tooling::getText(*E, *Match.Context);
+      return Error::success();
+    }
+    Source = tooling::buildDereference(*E, *Match.Context);
+    break;
+  case UnaryNodeOperator::AddressOf:
+    Source = tooling::buildAddressOf(*E, *Match.Context);
+    break;
+  case UnaryNodeOperator::MaybeAddressOf:
+    if (E->getType()->isAnyPointerType()) {
+      *Result += tooling::getText(*E, *Match.Context);
+      return Error::success();
+    }
     Source = tooling::buildAddressOf(*E, *Match.Context);
     break;
   }
   if (!Source)
     return llvm::make_error<StringError>(
         errc::invalid_argument,
         "Could not construct expression source from ID: " + Data.Id);
   *Result += *Source;
return std::make_shared<StencilImpl<UnaryOperationData>>(
       UnaryNodeOperator::Parens, Id);
 }
 
 Stencil transformer::deref(llvm::StringRef ExprId) {
   return std::make_shared<StencilImpl<UnaryOperationData>>(
       UnaryNodeOperator::Deref, ExprId);
 }
 
+Stencil transformer::maybeDeref(llvm::StringRef ExprId) {
+  return std::make_shared<StencilImpl<UnaryOperationData>>(
+      UnaryNodeOperator::MaybeDeref, ExprId);
+}
+
 Stencil transformer::addressOf(llvm::StringRef ExprId) {
   return std::make_shared<StencilImpl<UnaryOperationData>>(
-      UnaryNodeOperator::Address, ExprId);
+      UnaryNodeOperator::AddressOf, ExprId);
+}
+
+Stencil transformer::maybeAddressOf(llvm::StringRef ExprId) {
+  return std::make_shared<StencilImpl<UnaryOperationData>>(
+      UnaryNodeOperator::MaybeAddressOf, ExprId);
 }
 
 Stencil transformer::access(StringRef BaseId, Stencil Member) {
   return std::make_shared<StencilImpl<AccessData>>(BaseId, std::move(Member));
 }
 
 Stencil transformer::ifBound(StringRef Id, Stencil TrueStencil,
                              Stencil FalseStencil) {

clang/unittests/Tooling/StencilTest.cpp

TEST_F(StencilTest, AddressOfValue) {
   testExpr(Id, "int x; x;", addressOf(Id), "&x");
 }
 
 TEST_F(StencilTest, AddressOfDerefExpr) {
   StringRef Id = "id";
   testExpr(Id, "int *x; *x;", addressOf(Id), "x");
 }
 
+TEST_F(StencilTest, MaybeDerefValue) {
+  StringRef Id = "id";
+  testExpr(Id, "int x; x;", maybeDeref(Id), "x");
+}
+
+TEST_F(StencilTest, MaybeDerefPointer) {
+  StringRef Id = "id";
+  testExpr(Id, "int *x; x;", maybeDeref(Id), "*x");
+}
+
+TEST_F(StencilTest, MaybeDerefBinOp) {
+  StringRef Id = "id";
+  testExpr(Id, "int *x; x + 1;", maybeDeref(Id), "*(x + 1)");
+}
+
+TEST_F(StencilTest, MaybeDerefAddressExpr) {
+  StringRef Id = "id";
+  testExpr(Id, "int x; &x;", maybeDeref(Id), "x");
+}
+
+TEST_F(StencilTest, MaybeAddressOfPointer) {
+  StringRef Id = "id";
+  testExpr(Id, "int *x; x;", maybeAddressOf(Id), "x");
+}
+
+TEST_F(StencilTest, MaybeAddressOfValue) {
+  StringRef Id = "id";
+  testExpr(Id, "int x; x;", addressOf(Id), "&x");
+}
+
+TEST_F(StencilTest, MaybeAddressOfBinOp) {
+  StringRef Id = "id";
+  testExpr(Id, "int x; x + 1;", maybeAddressOf(Id), "&(x + 1)");
+}
+
+TEST_F(StencilTest, MaybeAddressOfDerefExpr) {
+  StringRef Id = "id";
+  testExpr(Id, "int *x; *x;", addressOf(Id), "x");
+}
+
 TEST_F(StencilTest, AccessOpValue) {
   StringRef Snippet = R"cc(
     S x;
     x;
   )cc";
   StringRef Id = "id";
   testExpr(Id, Snippet, access(Id, "field"), "x.field");
 }