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 Value Type Out Parameters #1772

Merged
merged 2 commits into from
Oct 18, 2023
Merged

Conversation

Saalvage
Copy link
Contributor

@Saalvage Saalvage commented Oct 18, 2023

Currently, using value types as out parameters generates incorrect code:

Given

union MyCoolUnion
{
    int a;
    float b;
};

int DLL_API DoCoolThing(CS_OUT MyCoolUnion* myCoolUnion);

the following code is generated:

public static void DoCoolThing(out global::Test.MyCoolUnion myCoolUnion)
{
    myCoolUnion = new global::Test.MyCoolUnion();
    var ____arg0 = new global::Test.MyCoolUnion.__Internal();
    var __arg0 = new __IntPtr(&____arg0);
    __Internal.DoCoolThing(__arg0);
}

This does not use the out parameter's internal value, but a newly created one, leaving the out parameter unchanged.

Following this PR the following code is generated:

public static void DoCoolThing(out global::Test.MyCoolUnion myCoolUnion)
{
    myCoolUnion = new global::Test.MyCoolUnion();
    fixed (global::Test.MyCoolUnion.__Internal* ____arg0 = &myCoolUnion.__Instance)
    {
    var __arg0 = new __IntPtr(____arg0);
    testerB = new global::Test.MyCoolUnion();
    __Internal.DoCoolThing(__arg0);
    }
}

Considerations:

  • The indentation is broken, the insides of the fixed statements are not being indented. I hope this is either 1. negligible or 2. easily fixable with greater understanding of the library.
  • Instead of using fixed a local could be used and the result later copied into the out parameter. I assume this would be more expensive, as it'd require an extra copy of a potentially very large structure. (If this is a big concern, it should probably be benchmarked)
  • The line Context.Return.Write($"new {typePrinter.IntPtrType}(&{arg})"); is duplicated once, with only the & being omitted. I'm currently not seeing a clean solution, but there might be one.

@tritao
Copy link
Collaborator

tritao commented Oct 18, 2023

Cool, thanks for the fix!

@tritao tritao merged commit 20950c5 into mono:main Oct 18, 2023
5 checks passed
@Saalvage Saalvage deleted the fix/value-type-out-param branch October 18, 2023 11:38
@Saalvage
Copy link
Contributor Author

Thank you for the quick merge! Are there any estimates on when there'll be another nuget release? I would like to use this fix in a project of mine but would want to avoid having to deal with git submodule or nuget shenanigans.

@tritao
Copy link
Collaborator

tritao commented Oct 18, 2023

Thank you for the quick merge! Are there any estimates on when there'll be another nuget release? I would like to use this fix in a project of mine but would want to avoid having to deal with git submodule or nuget shenanigans.

I've pushed a new tag, v1.1. Hopefully when the build ends a new version is published: https://github.com/mono/CppSharp/actions/runs/6560441820

@Saalvage
Copy link
Contributor Author

Thank you very much!!

@tritao
Copy link
Collaborator

tritao commented Oct 18, 2023

Should be up now.

https://github.com/mono/CppSharp/releases/tag/v1.1
https://www.nuget.org/packages/CppSharp/1.0.76.8341

There is some kind of mismatch in the package/release version name, will have to figure that out, but should be good enough to use for now.

@Saalvage
Copy link
Contributor Author

Hmmm, it doesn't seem to be working for me. The CppSharp namespace appears to just be missing with 1.0.76.8341, changing it to 1.0.54.626 makes it work as normal again.
Might be nuget taking a while to correctly serve the package??

@tritao
Copy link
Collaborator

tritao commented Oct 18, 2023

Hmmm, it doesn't seem to be working for me. The CppSharp namespace appears to just be missing with 1.0.76.8341, changing it to 1.0.54.626 makes it work as normal again. Might be nuget taking a while to correctly serve the package??

Not sure, everything looks fine when I download the package and check its contents. The process is the same that the previous release was created with so dunno.

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