Skip to content

"Winapi" support on NS20 and .NET 5 #561

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

Merged
merged 2 commits into from
Aug 7, 2021
Merged

"Winapi" support on NS20 and .NET 5 #561

merged 2 commits into from
Aug 7, 2021

Conversation

Perksey
Copy link
Member

@Perksey Perksey commented Aug 6, 2021

2.7 is due today, to get this fix in we'll delay release to tomorrow.

This implements support for platform-default P/Invoke calling conventions instead of blindly using Cdecl (which is incorrect!) This is something that was pointed out when SilkTouch development first started, but was not implemented.

On .NET Standard 2.0, seeing as there's no true way to specify "platform default" we use a "best effort" implementation to cover known cases. Right now, this is just 32-bit Windows being not Cdecl - there are no other concerns we know of today.

Codegen:

Copy link
Member

@HurricanKai HurricanKai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SilkTouch is run once per target.
Instead of delegating this to a runtime check that checks the Silk.NET.Core framework we built against, just put the #IF NET5_0 .... #else ... #endif right in the generated code (well not actually, just check in SilkTouch what target we are building).
Also, instead of having Winapi be the new default, just set it to be the default only for Assimp, that way we only have to multi-target Assimp to NET5 and NS.

I don't want to merge a PR that fixes an issue for x86 but breaks inlining everywhere else.

@Perksey
Copy link
Member Author

Perksey commented Aug 6, 2021

Instead of delegating this to a runtime check that checks the Silk.NET.Core framework we built against, just put the #IF NET5_0 .... #else ... #endif right in the generated code.

This uglies the code, both generated and generator and has zero gain. Not reworking just for this.

Also, instead of having Winapi be the new default, just set it to be the default only for Assimp, that way we only have to multi-target Assimp to NET5 and NS.

This is a deviation from DllImport behaviour and expected behaviour, but fine.

@HurricanKai
Copy link
Member

This uglies the code, both generated and generator and has zero gain. Not reworking just for this.
No it doesn't. It means there is zero code diff on .NET 5, and an extra if check on other platforms.

@Perksey
Copy link
Member Author

Perksey commented Aug 6, 2021

No it doesn't. It means there is zero code diff on .NET 5, and an extra if check on other platforms.

There already is zero generated code diff on .NET 5.

@Perksey
Copy link
Member Author

Perksey commented Aug 6, 2021

Unless you mean diff between the outputs of the two different targets, in which case I'm not reworking this for that as it's absolutely zero gain.

@Perksey Perksey requested a review from HurricanKai August 7, 2021 14:25
@Perksey
Copy link
Member Author

Perksey commented Aug 7, 2021

Is there anything left to do here?

Copy link
Member

@HurricanKai HurricanKai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not very happy with this, but I'll merge it for 2.7

@Perksey Perksey merged commit 9cdafd7 into main Aug 7, 2021
@Perksey Perksey deleted the feature/winapi branch August 7, 2021 22:49
@Perksey
Copy link
Member Author

Perksey commented Aug 7, 2021

For the record doing your ifdef in the generated code tactic has no real positives and quite a few negatives for example at this scale it will decrease performance doing this check at reparse time rather than generation time in addition to all of my reasons outlined above. You would be going out of your way to increase the compiler's load purely because you want the output to be the same between the target frameworks, which it effectively isn't anyway given disabled preprocessor branches are just text which the compiler discards - so really it's just a courtesy for... I have no idea who?

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

Successfully merging this pull request may close these issues.

2 participants