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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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