Skip to content

[FIX] is() function to handle polymorphic sidecast #128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 11, 2022

Conversation

filipsajdak
Copy link
Contributor

@filipsajdak filipsajdak commented Nov 22, 2022

The current implementation is not handling sidecasts.

The code:

struct B1 { virtual ~B1() = default; };

struct B2 { virtual ~B2() = default; };

struct D : B1, B2 {};

main: () -> int = {
    d: D = ();
    p: * B1 = d&;

    std::cout << p* is B2 << std::endl;
}

returns false - the current implementation of is() function is using std::is_base_of that blocks using dynamic_cast for sidecast. The current fix removes the requirement for B2 being a base for B1. The current fix add a check if we are dealing with polymorpic types - if yes then the dynamic_cast is called.

This PR close is() part of the #127

as() will be fixed after finalizing discussion here: #106 (it requires handling case when dynamic_cast will return nullptr or throw bad_cast).

@filipsajdak
Copy link
Contributor Author

That fix is currently not good... removing std::is_base_of_v<X, C> && makes it ambiguous.

I am changing

template< typename C, typename X >
requires (std::is_base_of_v<X, C> && !std::is_same_v<C,X>)
auto is( X const& x ) -> bool {
     return dynamic_cast<C const*>(&x) != nullptr;
}

to

template< typename C, typename X >
    requires (
        ( std::is_base_of_v<X, C> || 
          ( std::is_polymorphic_v<C> && std::is_polymorphic_v<X>) 
        ) && !std::is_same_v<C,X>)
auto is( X const& x ) -> bool {
    return dynamic_cast<C const*>(&x) != nullptr;
}

That will make the code unambiguous and will solve the issue.

@filipsajdak filipsajdak force-pushed the fsajdak-fix-is-for-sidecast branch from 2756e85 to b2ef5b3 Compare November 28, 2022 21:55
@filipsajdak filipsajdak force-pushed the fsajdak-fix-is-for-sidecast branch from b2ef5b3 to afb76c9 Compare November 29, 2022 22:28
@hsutter
Copy link
Owner

hsutter commented Dec 3, 2022

Thanks. Interim ack: I'm going to hold off on this and other changes to cpp2util.h for a short time, including most/all of the is and as fixes, because cpp2util.h is not yet covered by the current regression tests. (The current tests only cover regressions in .cpp lowering, and don't cover actually executing the resulting programs which is what exercises the code in cpp2util.h.)

I'm about to add support for actually executing the regression test programs (in at least one compiler, three would be ideal) and check in those results. Then they'll cover the cpp2util.h fixes too and we can work through those with some automated support for knowing we aren't getting regressions.

@filipsajdak
Copy link
Contributor Author

OK, that sounds good. I am also struggling with ensuring what is working or not. I am supporting myself with a compiler explorer to ensure if new ideas are working on gcc, clang, and MSVC.

@hsutter
Copy link
Owner

hsutter commented Dec 11, 2022

Looks good! Thanks.

@hsutter hsutter merged commit 4fa9e48 into hsutter:main Dec 11, 2022
@filipsajdak filipsajdak deleted the fsajdak-fix-is-for-sidecast branch December 22, 2022 10:43
Azmah-Bad pushed a commit to Azmah-Bad/cppfront that referenced this pull request Feb 24, 2023
…decast

[FIX] is() function to handle polymorphic sidecast
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants