Skip to content

Fix Value Type Out Parameters#1772

Merged
tritao merged 2 commits intomono:mainfrom
Saalvage:fix/value-type-out-param
Oct 18, 2023
Merged

Fix Value Type Out Parameters#1772
tritao merged 2 commits intomono:mainfrom
Saalvage:fix/value-type-out-param

Conversation

@Saalvage
Copy link
Copy Markdown
Contributor

@Saalvage Saalvage commented Oct 18, 2023 •
edited
Loading

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
Copy Markdown
Collaborator

tritao commented Oct 18, 2023

Cool, thanks for the fix!

@tritao tritao merged commit 20950c5 into mono:main Oct 18, 2023
@tritao tritao mentioned this pull request Oct 18, 2023
Closed
@Saalvage Saalvage deleted the fix/value-type-out-param branch October 18, 2023 11:38
@Saalvage
Copy link
Copy Markdown
Contributor Author

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

@tritao
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Saalvage commented Oct 18, 2023

Thank you very much!!

@tritao
Copy link
Copy Markdown
Collaborator

tritao commented Oct 18, 2023 •
edited
Loading

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
Copy Markdown
Contributor Author

Saalvage 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??

@tritao
Copy link
Copy Markdown
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.

@Saalvage Saalvage mentioned this pull request Oct 18, 2023
Closed
@JordanL8 JordanL8 mentioned this pull request Feb 16, 2024
Open
This was referenced Feb 6, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Saalvage @tritao