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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
