[PATCH] D116377: [libTooling] Adds more support for constructing object access expressions.

Dmitri Gribenko via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 17 01:52:55 PST 2022

gribozavr2 accepted this revision.
gribozavr2 added inline comments.
This revision is now accepted and ready to land.

Comment at: clang/lib/Tooling/Transformer/SourceCodeBuilders.cpp:78
+                              returns(qualType(references(type()))))));
+  const auto SmartPointer = qualType(hasDeclaration(
+      cxxRecordDecl(anyOf(KnownSmartPointer, QuacksLikeASmartPointer))));
asoffer wrote:
> I think std::optional satisfies these requirements but should not be considered a smart-pointer.
Are you objecting to naming, or to this code handling optionals and smart pointers in the same way at all?

An optional is not a pointer, however it has few semantic differences from `std::unique_ptr` that are relevant here (whether the object is heap-allocated or not is not relevant for building an access expression). So I think this code should handle optionals and other value wrappers like `llvm::Expected` and `absl::StatusOr`.

Comment at: clang/lib/Tooling/Transformer/SourceCodeBuilders.cpp:221-222
+      isSmartPointerType(E->getType(), Context)) {
+    // Strip off any operator->. This can only occur inside an actual arrow
+    // member access, so we treat it as equivalent to an actual object
+    // expression.

Comment at: clang/lib/Tooling/Transformer/SourceCodeBuilders.cpp:234
+  if (const auto *Operand = isSmartDereference(*E, Context)) {
+    // `buildDot` already handles the built-in dereference operator, so we
+    // only need to catch overloaded `operator*`.

Comment at: clang/unittests/Tooling/SourceCodeBuildersTest.cpp:141
+  std::string Snippet = "std::unique_ptr<int> P; P;";
+  auto StmtMatch = matchStmt(Snippet, expr(hasType(qualType().bind("ty"))));
+  ASSERT_TRUE(StmtMatch) << "Snippet: " << Snippet;
There are actually two expressions in the snippet above, one is the `DeclRefExpr` in `P;`, but the other is the `CXXConstructExpr` in `std::unique_ptr<int> P;`. Both have the same type, so it makes no difference which one we find, but the snippet deliberately having `P;` makes it look like we specifically want the `DeclRefExpr`. I'd suggest changing the matcher to `declRefExpr()` for clarity. Same in tests below.

Comment at: clang/unittests/Tooling/SourceCodeBuildersTest.cpp:373
+TEST(SourceCodeBuildersTest, BuildAccessSmartPointer) {
+  testBuilder(buildAccess, "Smart x; x;", "x->");
This is a case where the old APIs provided the user with a choice, but the new API does not. If the user wanted to call a method on the smart pointer itself (e.g., `reset()`), they could have used `buildDot` to get `x.`.

IDK if it is an important use case, but I thought I'd mention it, since the new API is not 100% equivalent to the ones that it replaces.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list