dynamic_cast<> with pointers instead of reference and compare against NU... #87

Merged
merged 1 commit into from Aug 7, 2013

3 participants

@DavidKeller

Rework the down cast to use dynami_cast<> on pointers + comparison against NULL instead of dynamic_cast on references + std::bad_cast catch.

This is faster and safer:
1 comparison
VS
1 comparison and maybe (1 throw and 1 catch)

@DavidKeller DavidKeller commented on the diff Jul 24, 2013
ext/common/ApplicationPool2/Implementation.cpp
#define TRY_COPY_EXCEPTION(klass) \
do { \
- if (exceptionIsInstanceOf<const klass &>(e)) { \
- return make_shared<klass>( (const klass &) e ); \

This (const klass&) cast can be dangerous when virtual inheritance is involved (as it should with exception classes, http://www.boost.org/doc/libs/1_53_0/libs/exception/doc/using_virtual_inheritance_in_exception_types.html).

@FooBarWidget
Phusion B.V. member

That is fine. We only check for a limited number of classes here, none of which use multiple inheritance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@FooBarWidget
Phusion B.V. member

Interesting, I didn't know that that would actually work as a workaround. I'm currently in the middle of some heavy tax administration work, but I'll get around to test your pull request as soon as I can. Thank you for submitting this patch.

In the mean time, could you sign our contributor agreement? We need such an agreement prior to merging contributions because we'd like this to become available in Phusion Passenger Enterprise as well.

@DavidKeller

Sure, I've accepted it using pgp signing :-)

@dvisor

Heads up @DavidKeller - thus far looks like your patch works for me. Still working out kinks getting our app working in a new ruby20/rails4 env, but test pages seem to serve correctly.

Thanks for this contribution!

@FooBarWidget FooBarWidget merged commit 153397a into phusion:master Aug 7, 2013

1 check passed

Details default The Travis CI build passed
@FooBarWidget
Phusion B.V. member

Thanks for the patch DavidKeller. It has been merged and I've added you to the contributors list.

@DavidKeller

You're welcome.

Regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment