Page MenuHomePhabricator

private_typeinfo: limit is_dst_type_derived_from_static_type optimization
ClosedPublic

Authored by rprichard on Aug 8 2017, 12:35 AM.

Details

Reviewers
EricWF
Summary

If the destination type does not derive from the static type, we can skip
the search_above_dst call, but we still need to run the
!does_dst_type_point_to_our_static_type block of code. That block of code
will increment info->number_to_dst_ptr to 2, and because dest isn't derived
from static, the cast will ultimately fail.

Fixes PR33439

Diff Detail

Repository
rL LLVM

Event Timeline

rprichard created this revision.Aug 8 2017, 12:35 AM
grandinj added inline comments.
src/private_typeinfo.cpp
911 ↗(On Diff #110147)

static_type,s
->
static_type's

914 ↗(On Diff #110147)

it if it
->
it, if it

1048 ↗(On Diff #110147)

ditto

1051 ↗(On Diff #110147)

ditto

rprichard updated this revision to Diff 110292.Aug 8 2017, 3:27 PM
rprichard marked 4 inline comments as done.
rprichard marked an inline comment as not done.

Incorporate cosmetic changes to comments.

rprichard marked an inline comment as done.Aug 8 2017, 3:30 PM

See https://reviews.llvm.org/D36449 for a set of regression tests.

EricWF requested changes to this revision.Sep 12 2017, 6:58 PM

Could you please include the tests for this change in this review?

Feel free to make this review dependent on D36446 or vice versa.

This revision now requires changes to proceed.Sep 12 2017, 6:58 PM
rprichard updated this revision to Diff 118154.Oct 8 2017, 1:32 AM
rprichard edited edge metadata.

Consolidate the regression test with the bugfix.

rprichard updated this revision to Diff 118159.Oct 8 2017, 2:03 AM

Ping about this patch.

EricWF accepted this revision.May 12 2018, 6:48 PM

This also LGTM. Please make sure to correct the new blocks are properly indented before committing.

This revision is now accepted and ready to land.May 12 2018, 6:48 PM

This also LGTM. Please make sure to correct the new blocks are properly indented before committing.

Thanks for the review. What blocks need an indentation fix?

Looking at the actual raw diff, it looks like it's indented correctly - but phab's being a little weird about how it's displaying it.
I couldn't actually apply the phab patch directly, so I had to recreate it. So I'm not talking about any problem in specific.

Looking at the actual raw diff, it looks like it's indented correctly - but phab's being a little weird about how it's displaying it.
I couldn't actually apply the phab patch directly, so I had to recreate it. So I'm not talking about any problem in specific.

Sounds good to me. I don't have commit access -- who should commit these changes?

Looking at the actual raw diff, it looks like it's indented correctly - but phab's being a little weird about how it's displaying it.
I couldn't actually apply the phab patch directly, so I had to recreate it. So I'm not talking about any problem in specific.

Sounds good to me. I don't have commit access -- who should commit these changes?

Sorry I should have asked if you have commit access. I'll commit these on your behalf.

EricWF closed this revision.May 18 2018, 1:55 PM

Committed as r332767.