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

Generic math #15

Merged
merged 7 commits into from
Aug 30, 2023
Merged

Generic math #15

merged 7 commits into from
Aug 30, 2023

Conversation

Arasfon
Copy link
Contributor

@Arasfon Arasfon commented Aug 30, 2023

Works only with .NET 7 TFM.
Why: All frameworks older than .NET 7 do not support generic math in any way as far as I know as I said in #11.

Replaced sizeof(int) with sizeof(long) in stackalloc bounds calculation, because it is impossible to calculate sizeof(T) without unsafe. However if unsafe is not a bad thing to add, sizeof(T) can be added.

Important: Removed all target frameworks except .NET 7. Removed `stackalloc`, because it is impossible to use it with T.
`sizeof(T)` is impossible, so `sizeof(long)` was chosen as a fallback.
src/Sqids/SqidsEncoder.cs Outdated Show resolved Hide resolved
src/Sqids/SqidsEncoder.cs Outdated Show resolved Hide resolved
@aradalvand
Copy link
Collaborator

aradalvand commented Aug 30, 2023

Thank you, @Arasfon

Removed all target frameworks except .NET 7. Why: All frameworks older than .NET 7 do not support generic math in any way as far as I know as I said in #11.

It's better to preserve support for older frameworks while having them only support int rather than drop support for them completely, and then use generic math specifically in .NET 7 onwards. I think you can use preprocessor directives like #if NET7_0 to achieve that. But removing support for older frameworks entirely isn't acceptable.

@aradalvand
Copy link
Collaborator

aradalvand commented Aug 30, 2023

See the docs.
You need to do something like this — e.g.:

#if NET7_0_OR_GREATER
public T[] Decode(string id) => Decode(id.AsSpan());
#else
public int[] Decode(string id) => Decode(id.AsSpan());
#endif

Generic math only for .NET 7 or later, and previous implementation for other frameworks.
@Arasfon
Copy link
Contributor Author

Arasfon commented Aug 30, 2023

@aradalvand, Check latest commit, everything is under preprocessor directives now.

@aradalvand
Copy link
Collaborator

@Arasfon Great, thanks!
Could you also add some tests for encoding/decoding numbers of types other than int? Like long, byte, ulong, etc.

@Arasfon
Copy link
Contributor Author

Arasfon commented Aug 30, 2023

@aradalvand I have added tests for both single and multiple numbers. Check them out

Added because it depends on SqidsEncoder.
@Arasfon Arasfon marked this pull request as ready for review August 30, 2023 12:30
@Arasfon Arasfon mentioned this pull request Aug 30, 2023
src/Sqids/SqidsEncoder.cs Outdated Show resolved Hide resolved
@celluj34
Copy link

I wonder if source generators would be a better way to accomplish this? Then we could output only int version for .Net 6 and below, and all number versions for .Net 7 and above, and we could re-use all of the code.

@aradalvand
Copy link
Collaborator

aradalvand commented Aug 30, 2023

I wonder if source generators would be a better way to accomplish this? Then we could output only int version for .Net 6 and below, and all number versions for .Net 7 and above, and we could re-use all of the code.

This isn't really the type of scenario you would use source generators for. That would introduce far too much complexity for something that simple preprocessor directives are currently accomplishing. And we already are reusing most of the code; so that's not a concern.

@aradalvand aradalvand merged commit 59812c2 into sqids:main Aug 30, 2023
1 check passed
@aradalvand
Copy link
Collaborator

Thank you @Arasfon for the contribution! And thanks @celluj34 for your participation!

I'll need to take care of this and then I'll release 2.0

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.

Add support for long (and potentially other integral types)
3 participants