Skip to content

Conversation

Uralstech
Copy link
Contributor

Overview:
This pull request enhances framework compatibility of LLamaSharp, addressing some issues discussed in #178.

Changes Made:

  • Updated preprocessor directives in the LLamaSharp C# project to ensure compatibility with .NET Standard 2.0 and higher and .NET 6.0 and higher.
  • Additionally, preprocessor errors have been introduced to provide clear feedback when attempting to target a possibly unsupported framework.

@martindevans
Copy link
Member

Thanks for the PR!

I don't think the approach taken here it quite right though. The extension methods covered with #if NETSTANDARD2_0 are usually polyfills for methods which were introduced in later versions of dotnet. That means they should be defined on platforms where those methods don't exist, but must not exist on platforms where those methods already exist.

You could probably use #if !NETSTANDARD2_1 for most of the blocks I would guess?

@Uralstech
Copy link
Contributor Author

Uralstech commented Nov 6, 2023

I don't think the approach taken here it quite right though. The extension methods covered with #if NETSTANDARD2_0 are usually polyfills for methods which were introduced in later versions of dotnet. That means they should be defined on platforms where those methods don't exist, but must not exist on platforms where those methods already exist.

You could probably use #if !NETSTANDARD2_1 for most of the blocks I would guess?

I understand. In my previous testing with Unity, I had found EnsureCapacity<T>(this List<T> list, int capacity) is not defined, so I thought it would be better to update all instances of NETSTANDARD2_0.

I'll just have EnsureCapacity with NETSTANDARD2_0 || NETSTANDARD2_1 then!

Edit: I just updated it!

@martindevans
Copy link
Member

Thanks for fixing that so quickly, just a couple of other minor nits I spotted in review :)

@martindevans
Copy link
Member

Looks good to me! Just waiting for those tests to run and I'll merge it. Thanks very much :)

@Uralstech
Copy link
Contributor Author

Thank you! You're welcome!

@martindevans martindevans merged commit 73ed399 into SciSharp:master Nov 7, 2023
@Uralstech Uralstech deleted the multi-targeting-netstandard branch November 7, 2023 14:07
martindevans added a commit to martindevans/LLamaSharp that referenced this pull request Nov 15, 2023
…hat it shouldn't. Fixed that.

Originally from these PRs:
 - SciSharp#263
 - SciSharp#259
@martindevans martindevans mentioned this pull request Nov 15, 2023
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