ymandel added inline comments.
================
Comment at: clang/lib/Tooling/Transformer/SourceCodeBuilders.cpp:78
+ returns(qualType(references(type()))))));
+ const auto SmartPointer = qualType(hasDeclaration(
+ cxxRecordDecl(anyOf(KnownSmartPointer, QuacksLikeASmartPointer))));
----------------
gribozavr2 wrote:
> 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`.
I went with a strict list of known names, but expanded the list as specified in
the comments. Also, I explicitly note that API doesn't guarantee stability of
that list (over time).
================
Comment at: clang/unittests/Tooling/SourceCodeBuildersTest.cpp:71
+ auto AstUnit = buildASTFromCodeWithArgs(wrapSnippet(StatementCode),
+ {"-Wno-unused-value"});
if (AstUnit == nullptr) {
----------------
drive-by fix to silence distracting warnings in the tests.
================
Comment at: clang/unittests/Tooling/SourceCodeBuildersTest.cpp:373
+TEST(SourceCodeBuildersTest, BuildAccessSmartPointer) {
+ testBuilder(buildAccess, "Smart x; x;", "x->");
+}
----------------
gribozavr2 wrote:
> ymandel wrote:
> > gribozavr2 wrote:
> > > 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.
> > Agreed. I think it is important and I should add a (defaulted) parameter to
> > `buildAccess` that forces "smart" pointers to be treated like any other
> > value. WDYT?
> Yes, something like AllowDereferencingSmartPointers.
went with `PLTClass` for an enum name, but happy to take alternative
suggestions (see the header file).
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
