Page MenuHomePhabricator

Update `DynTypedNode` to support the conversion of `TypeLoc`s.
ClosedPublic

Authored by jcking1034 on Sep 27 2021, 1:43 PM.

Details

Summary

This provides better support for TypeLocs to allow TypeLoc-related
matchers to feature stricter typing and to avoid relying on the dynamic
casting of TypeLocs in matchers.

Diff Detail

Event Timeline

jcking1034 requested review of this revision.Sep 27 2021, 1:43 PM
jcking1034 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2021, 1:44 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jcking1034 retitled this revision from Update `DynTypedNode` to support the conversion of TypeLocs. to Update `DynTypedNode` to support the conversion of `TypeLoc`s..Sep 27 2021, 1:55 PM
jcking1034 edited the summary of this revision. (Show Details)

Looks good! Please add a test to be sure it compiles/works correctly. Thanks!

clang/include/clang/AST/ASTTypeTraits.h
57

Here and below -- looks like clang format did this? I'd recommend you revert this line and others like it -- only change formatting on lines you've otherwise edited.

sbenza added inline comments.Sep 30 2021, 10:36 AM
clang/include/clang/AST/ASTTypeTraits.h
476

The create/get don't seem to match.
We are constructing a BaseT object in create(), so this static_cast to a derived type is UB?
That memory does not contain a T.

If we instead construct a T in create(), we should be able to read it as a BaseT* (assuming is_pointer_interconvertible_base_of<BaseT, T> is true)

Restore original formatting on unchanged lines.

jcking1034 marked an inline comment as done.Sep 30 2021, 11:38 AM

Construct an object of the derived type, instead of the base type.

jcking1034 marked an inline comment as done.Sep 30 2021, 3:44 PM
jcking1034 added inline comments.
clang/include/clang/AST/ASTTypeTraits.h
476

Done, constructing a T instead of a BaseT seems to work!

jcking1034 marked an inline comment as done.

Fix typing for create and add unit tests.

ymandel accepted this revision.Oct 1 2021, 11:05 AM
ymandel added inline comments.
clang/unittests/AST/ASTTypeTraitsTest.cpp
207

let's assert that matches.size() == 1 and then just use nodes[0].

This revision is now accepted and ready to land.Oct 1 2021, 11:05 AM
tdl-g accepted this revision.Oct 1 2021, 11:41 AM

Clean up unit tests.

jcking1034 marked an inline comment as done.Oct 1 2021, 12:49 PM
sbenza added inline comments.Oct 1 2021, 1:30 PM
clang/unittests/AST/ASTTypeTraitsTest.cpp
212

I don't know what we are trying to check with this self equivalence.
Did you mean Node == tl?

225

We should check that we can do getNodeAs<PointerTypeLoc> and getNodeAs<TypeLoc>.
That is the goal of having the hierarchy in DynTypedNode.

jcking1034 updated this revision to Diff 376901.Oct 4 2021, 8:11 AM
jcking1034 marked an inline comment as done.

Update unit test to better exercise getNodeAs

jcking1034 marked an inline comment as not done.Oct 4 2021, 8:12 AM
jcking1034 added inline comments.
clang/unittests/AST/ASTTypeTraitsTest.cpp
212

These two EXPECTs are meant to ensure that the overloaded equality and less-than operators do function correctly when handling nodes of the TypeLoc family. In both cases, there is behavior specifically for these nodes, and I've made minor updates to both (changing isSame to isBaseOf). Additionally, this may give a little more confidence that template specialization functioned correctly. I will note that the QualType test performs a similar set of checks, presumably for similar reasoning.

sbenza accepted this revision.Oct 4 2021, 9:34 AM
sbenza added inline comments.
clang/unittests/AST/ASTTypeTraitsTest.cpp
234

Maybe also check

EXPECT_EQ(&tl, &ptl);

?

Include additional expectation in unit test

jcking1034 marked an inline comment as done.Oct 4 2021, 10:13 AM

Fix pointer style

This revision was landed with ongoing or failed builds.Oct 4 2021, 12:26 PM
This revision was automatically updated to reflect the committed changes.