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

gint64 array length parameters cause invalid cast error #1098

Closed
adamreeve opened this issue Jul 19, 2024 · 4 comments · Fixed by #1126
Closed

gint64 array length parameters cause invalid cast error #1098

adamreeve opened this issue Jul 19, 2024 · 4 comments · Fixed by #1126
Labels
bug Something isn't working
Milestone

Comments

@adamreeve
Copy link
Contributor

Since #1067 was merged, I now get errors building bindings for methods where the array length has type gint64. I've pushed a change to the GirTest library to reproduce the error here: adamreeve@f93b90f

The generated method looks like:

public static int GetDataWithGint64Size(Span<int> data)
{
    var resultGetDataWithGint64Size = GirTest.Internal.IntegerArrayTester.GetDataWithGint64Size(ref MemoryMarshal.GetReference(data), (long) data.Length);
    return resultGetDataWithGint64Size;
}

The expected type of the length parameter to the internal method is CLong rather than long, which causes errors when building the generated code like:

error CS1503: Argument 2: cannot convert from 'long' to 'System.Runtime.InteropServices.CLong'

This appears to be due to the PrimitiveValueTypeArray implementation of ToNativeParameterConverter here:

lengthParameter.SetCallName(() => $"({type}) {parameterName}.Length");

But I haven't yet figured out how best to fix this. I guess we want to reuse the Long instance of ToNativeParameterConverter rather than hard code the cast here?

@badcel badcel added this to the 0.6.0 milestone Jul 19, 2024
@badcel badcel added the bug Something isn't working label Jul 19, 2024
@badcel
Copy link
Member

badcel commented Jul 19, 2024

Hey, thanks for the report. I did not yet take a detailed look at this bug but it can be that there is no easy fix for it and needs updating the typehandling of the generator.

Gint64 is guaranteed to be 64 bits on all platforms.

The GirLoader maps Gint64 to long.

The GirModel.Long is currently mapped to CLong in the internal APIs as the C long is only 32bit on windows and 64Bit on other platforms.

This is obviously wrong for GInt64 as it has 64Bit on all platforms. So either there must be a new Type like Long64 or a property must be added to Long detailing whether it is always 64 bit or not.

I need to take a more detailed look here.

@adamreeve
Copy link
Contributor Author

Ah good point, yeah gint64 should definitely end up mapped to .NET long without going via CLong and being truncated to 32 bits on Windows.

badcel added a commit that referenced this issue Sep 25, 2024
GirModel.CLong is 32 bit on all windows platforms, 32 bit on 32 bit unix platforms and 64 bit on 64bit unix platforms. GirModel.Long ist 64 bit on all platforms.

This reverts the changes from #1067 and only adds the CLong / CULong handling for "glong" / "gulong". All other long types stay like originally intended.

Fixes #1098
badcel added a commit that referenced this issue Sep 26, 2024
GirModel.CLong is 32 bit on all windows platforms, 32 bit on 32 bit unix platforms and 64 bit on 64bit unix platforms. GirModel.Long ist 64 bit on all platforms.

This reverts the changes from #1067 and only adds the CLong / CULong handling for "glong" / "gulong". All other long types stay like originally intended.

Fixes #1098
badcel added a commit that referenced this issue Sep 26, 2024
GirModel.CLong is 32 bit on all windows platforms, 32 bit on 32 bit unix platforms and 64 bit on 64bit unix platforms. GirModel.Long ist 64 bit on all platforms.

This reverts the changes from #1067 and only adds the CLong / CULong handling for "glong" / "gulong". All other long types stay like originally intended.

Fixes #1098
@badcel badcel closed this as completed in 699e2a8 Sep 26, 2024
@badcel
Copy link
Member

badcel commented Sep 26, 2024

@adamreeve This should be fixed again. Thanks for reporting. I added your test scenario to the tests, too.

@adamreeve
Copy link
Contributor Author

👍 thanks for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants