This is an archive of the discontinued LLVM Phabricator instance.

[libTooling] Adds more support for constructing object access expressions.
ClosedPublic

Authored by ymandel on Dec 29 2021, 7:49 AM.

Details

Summary

This patch adds a buildAccess function, which constructs a string with the
proper operator to use based on the expression's form and type. It also adds two
predicates related to smart pointers, which are needed by buildAccess but are
also of general value.

Diff Detail

Unit TestsFailed

Event Timeline

ymandel requested review of this revision.Dec 29 2021, 7:49 AM
ymandel created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 29 2021, 7:49 AM
ymandel updated this revision to Diff 397356.Jan 4 2022, 11:33 AM

ignore implicit and add deprecations

asoffer added inline comments.Jan 7 2022, 7:50 AM
clang/lib/Tooling/Transformer/SourceCodeBuilders.cpp
73

Naming nit: This could be confusing for anyone not familiar with the "duck typing" idiom.
BehavesLikeASmartPointer?

78

I think std::optional satisfies these requirements but should not be considered a smart-pointer.

79

The known smart pointers quack like smart pointers, so this is redundant, right?

gribozavr2 accepted this revision.Jan 17 2022, 1:52 AM
gribozavr2 added inline comments.
clang/lib/Tooling/Transformer/SourceCodeBuilders.cpp
78

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.

221–222
234
clang/unittests/Tooling/SourceCodeBuildersTest.cpp
141

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.

373

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.

This revision is now accepted and ready to land.Jan 17 2022, 1:52 AM
ymandel marked an inline comment as done.Jan 17 2022, 7:23 AM
ymandel added inline comments.
clang/unittests/Tooling/SourceCodeBuildersTest.cpp
373

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?

gribozavr2 added inline comments.Jan 17 2022, 8:34 AM
clang/unittests/Tooling/SourceCodeBuildersTest.cpp
373

Yes, something like AllowDereferencingSmartPointers.

ymandel updated this revision to Diff 402663.Jan 24 2022, 2:14 PM
ymandel marked 9 inline comments as done.

addressed comments.

ymandel added inline comments.Jan 24 2022, 2:22 PM
clang/lib/Tooling/Transformer/SourceCodeBuilders.cpp
78

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).

clang/unittests/Tooling/SourceCodeBuildersTest.cpp
65

drive-by fix to silence distracting warnings in the tests.

373

went with PLTClass for an enum name, but happy to take alternative suggestions (see the header file).

ymandel updated this revision to Diff 402910.Jan 25 2022, 7:41 AM

extended list of pointer-like types

gribozavr2 accepted this revision.Jan 25 2022, 10:23 AM
ymandel updated this revision to Diff 402983.Jan 25 2022, 11:37 AM

Fix StencilTests to align with new semantics

Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
This revision now requires review to proceed.Jan 25 2022, 11:37 AM
ymandel updated this revision to Diff 402984.Jan 25 2022, 11:38 AM

fix bad previous snapshot

ymandel removed reviewers: nicolasvasilache, aartbik, MaskRay, Restricted Project.Jan 25 2022, 11:41 AM
ymandel removed projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project.
ymandel removed subscribers: JDevlieghere, emaste, mgorny and 60 others.
This revision is now accepted and ready to land.Jan 25 2022, 11:41 AM
This revision was landed with ongoing or failed builds.Jan 25 2022, 11:43 AM
This revision was automatically updated to reflect the committed changes.