Page MenuHomePhabricator

[libTooling] Add support for smart pointers to relevant Transformer `Stencil`s.
ClosedPublic

Authored by ymandel on Dec 21 2020, 7:58 AM.

Details

Summary

Stencils maybeDeref and maybeAddressOf are designed to handle nodes that may
be pointers. Currently, they only handle native pointers. This patch extends the
support to recognize smart pointers and handle them as well.

Diff Detail

Event Timeline

ymandel requested review of this revision.Dec 21 2020, 7:58 AM
ymandel created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2020, 7:58 AM
tdl-g added inline comments.Jan 4 2021, 6:05 AM
clang/unittests/Tooling/StencilTest.cpp
277

You're only testing the "QuacksLike" case. I suspect you should have tests that validate the "KnownSmartPointers".

Admittedly, it's a bit redundant since the known smart pointers also QuackLike pointers. Which, I guess, raises the question of why you have the hard-coded list of KnownSmartPointers if they are covered by the QuacksLike behavior and thus can't be meaningfully tested independently.

What do you think?

ymandel retitled this revision from [libTooling] Add support for smart pointers to releveant Transformer `Stencil`s. to [libTooling] Add support for smart pointers to relevant Transformer `Stencil`s..Jan 5 2021, 7:45 AM
ymandel updated this revision to Diff 314615.Jan 5 2021, 7:53 AM

added comment known smart pointers.

ymandel marked an inline comment as done.Jan 5 2021, 7:55 AM

Thanks for the review!

clang/unittests/Tooling/StencilTest.cpp
277

You're right -- there isn't really a meaningful test specific to the known cases. I added a comment explaining that hard-coding them is an optimization. WDYT?

tdl-g added a comment.Jan 5 2021, 8:27 AM

Ah, if it's just an optimization that makes sense. I still think it's worth having a test case to confirm that one of the specially-handled cases works.

ymandel updated this revision to Diff 314642.Jan 5 2021, 9:28 AM
ymandel marked an inline comment as done.

Added test for unique_ptr specifically.

Ah, if it's just an optimization that makes sense. I still think it's worth having a test case to confirm that one of the specially-handled cases works.

Sure. I added one for unique pointer. but, just one -- i didn't add one per case. My logic is that since it's not part of the contract/API, we're really just using the (single) test to test the internal function isSmartPointerType, so one test is enough. That said, if you disagree, I'm fine with adding more tests. Its easy.

tdl-g accepted this revision.Jan 5 2021, 9:42 AM
This revision is now accepted and ready to land.Jan 5 2021, 9:42 AM
This revision was landed with ongoing or failed builds.Jan 5 2021, 9:57 AM
This revision was automatically updated to reflect the committed changes.