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

[C#]SpanT is available in .Net Standard 2.0. #6137

Merged
merged 1 commit into from
Sep 25, 2020
Merged

[C#]SpanT is available in .Net Standard 2.0. #6137

merged 1 commit into from
Sep 25, 2020

Conversation

harujoh
Copy link
Contributor

@harujoh harujoh commented Sep 25, 2020

Restored the use of SpanT in .NetStandard 2.0 as pointed out in the following link.

#6073 (comment)

A warning is added for compiling if the flags are inconsistent.

@harujoh harujoh changed the title SpanT is available in .Net Standard 2.0. [C#]SpanT is available in .Net Standard 2.0. Sep 25, 2020
@dbaileychess dbaileychess merged commit 52e3628 into google:master Sep 25, 2020
@anassinator
Copy link
Contributor

Thanks, this fixed it!

@harujoh harujoh deleted the SpanT_Available_In_NetStandard2.0 branch September 25, 2020 22:45
@ConanChenTookMyName
Copy link
Contributor

To my knowledge, Span<T> isn't available in .NET Standard 2.0, unless the "System.Memory" NuGet package is added to the project. I think FlatBuffers tries to avoid having additional dependencies? Your previous pull request actually had a discussion with @dbaileychess on why you decided to restrict ENABLE_SPAN_T to .NET Standard 2.1 before.

The current .NET Core project, as-is, doesn't build successfully when targeting .NET Standard 2.0 with UNSAFE_BYTEBUFFER and ENABLE_SPAN_T defined, when I tried.

Perhaps @anassinator can help clarify?

@anassinator
Copy link
Contributor

@ConanChenTookMyName We do use the System.Memory package with .NET Standard 2.0. I don't think FlatBuffers doesn't need to force that dependency on users, but there's already a precedent for supporting optional dependencies in the FlatBuffers python library for example (specifically numpy). So I don't think we should break that support especially since users have to opt in to use UNSAFE_BYTEBUFFER in the first place.

@ConanChenTookMyName
Copy link
Contributor

@anassinator Are you using a custom/modified project file to include the package reference?

@anassinator
Copy link
Contributor

@ConanChenTookMyName Yes, we have our own separate build system.

@harujoh
Copy link
Contributor Author

harujoh commented Sep 26, 2020

@ConanChenTookMyName I also decided that supporting .Net Standard 2.0 was unnecessary because the project lacked the necessary references and was not as fast as it should be.

But there were potential users like him, and perhaps he was in trouble. And he's probably not the only one in trouble.

The reinstatement of limited support for .NetStandard 2.0 is not detrimental to current performance. I also think it is in line with @dbaileychess desire to "widely available as possible".

I also had some problems with my commits.
Under NetStandard 2.0, using the ENABLE_SPAN_T flag will not compile.

This is because in some environments (e.g. Visual Studio), the code is compiled in parallel with 2.1, so I think there were cases where 2.1 compiling didn't even go through due to 2.0 errors.

This commit takes that into account, and prints a warning rather than an error for the wrong flag combination.

ivannp pushed a commit to ivannp/flatbuffers that referenced this pull request Oct 2, 2020
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.

4 participants