Skip to content
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

Fix variant call compiler error #1017

Closed
wants to merge 1 commit into from

Conversation

mashumafi
Copy link
Contributor

@mashumafi mashumafi commented Jan 28, 2023

Fixes #1015, the compiler gets confused because the function wasn't using Variant **. I tried to keep it as true to the original by using std::array but I had to rename functions like call -> callp.

@mashumafi mashumafi requested a review from a team as a code owner January 28, 2023 05:29
@akien-mga akien-mga added the bug This has been identified as a bug label Jan 28, 2023
@Fran6nd
Copy link

Fran6nd commented Feb 26, 2023

Any chance for this to be reviewed soon? As it is currently a blocking point regarding one of our projects... BTW I do know that everyone is busy with 1.0 stable coming soon.

@Trey2k
Copy link
Contributor

Trey2k commented Mar 13, 2023

This is currently a huge blocking point for me as well. Any chance of this getting reviewed soon?

@Fran6nd
Copy link

Fran6nd commented Mar 17, 2023

@akien-mga may I tag you here. This is critical for some plugin development.

@Trey2k
Copy link
Contributor

Trey2k commented May 17, 2023

Any update on this? This issue is still blocking for me, (well at this point I have started using a fork with this PR applied). But I would like to not have to do that if possible.

@dsnopek
Copy link
Contributor

dsnopek commented May 17, 2023

Unfortunately, I don't think anyone from the GDExtension team has had a chance to look at this yet. At the moment, we're really focused on finishing some things up before the 4.1 feature freeze in two week, but after that, we're going to switch to focusing on bug fixes. I've added this PR to my personal list of bugs to dig into!

@dsnopek
Copy link
Contributor

dsnopek commented Aug 12, 2023

Sorry for taking so long to come back to this!

I think your assessment that the compiler is getting confused between the two versions of Variant::call() is essentially correct. And renaming the first one to callp() seems like the right way to go - in Godot's variant.h, that method is also called callp(), and it also does a similar array conversion.

I'm a little worried about this PR needing to add the #include <algorithm>, though. variant.hpp is used by almost all of godot-cpp, and so almost all compilation units will now be pulling this in. I think I'd personally prefer this to use an approach closer to the variant.h from Godot, which doesn't use std::array, or atleast uses a loop rather than std::transform.

@dsnopek
Copy link
Contributor

dsnopek commented Sep 9, 2023

I just created PR #1238 which starts from the changes here, rebases, makes the requested changes from review, and adds a unit test

@mashumafi
Copy link
Contributor Author

Superseded by #1238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived bug This has been identified as a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compilation errors with Variant::call()
6 participants