FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[FIX] is() function to handle polymorphic sidecast by filipsajdak · Pull Request #128 · hsutter/cppfront · GitHub

Repository navigation

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

Merged
hsutter merged 1 commit into
hsutter:mainfrom
filipsajdak:fsajdak-fix-is-for-sidecast
Dec 11, 2022
Merged

hsutter merged 1 commit into
hsutter:mainfrom
filipsajdak:fsajdak-fix-is-for-sidecast

Conversation

filipsajdak commented Nov 22, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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).

Copy link
Copy Markdown
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 force-pushed the fsajdak-fix-is-for-sidecast branch from 2756e85 to b2ef5b3 Compare November 28, 2022 21:55
filipsajdak force-pushed the fsajdak-fix-is-for-sidecast branch from b2ef5b3 to afb76c9 Compare November 29, 2022 22:28

hsutter commented Dec 3, 2022

Copy link
Copy Markdown
Owner

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.

Copy link
Copy Markdown
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 commented Dec 11, 2022

Copy link
Copy Markdown
Owner

Looks good! Thanks.

hsutter merged commit 4fa9e48 into hsutter:main Dec 11, 2022
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL