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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
110 ms | x64 debian > LLVM.Bindings/Go::go.test |
Event Timeline
clang/lib/Tooling/Transformer/SourceCodeBuilders.cpp | ||
---|---|---|
73 | Naming nit: This could be confusing for anyone not familiar with the "duck typing" idiom. | |
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? |
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. |
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? |
clang/unittests/Tooling/SourceCodeBuildersTest.cpp | ||
---|---|---|
373 | Yes, something like AllowDereferencingSmartPointers. |
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). |
Naming nit: This could be confusing for anyone not familiar with the "duck typing" idiom.
BehavesLikeASmartPointer?