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

[Dmitri Gribenko via Phabricator via cfe-commits]

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116377/new/

https://reviews.llvm.org/D116377