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

fixed CS8352 in net8 #1121

Closed
wants to merge 1 commit into from
Closed

fixed CS8352 in net8 #1121

wants to merge 1 commit into from

Conversation

RomanKalachik
Copy link

@RomanKalachik RomanKalachik commented Jan 8, 2024

if you try to compile this project in net8, it will show some

SvgPointCollection.cs(66,89,66,94): error CS8352: Cannot use variable 'state' in this context because it may expose referenced variables outside of their declaration scope

This MR fixes that errors.

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

@paulushub
Copy link
Contributor

@RomanKalachik Thanks for the contribution. Still do not understand why ref was needed, but will look into it.
How about just adding the .NET 8 support as well? (was hoping #1019 will add it but still no progress there).

@RomanKalachik
Copy link
Author

@RomanKalachik Thanks for the contribution. Still do not understand why ref was needed, but will look into it. How about just adding the .NET 8 support as well? (was hoping #1019 will add it but still no progress there).

Net8.0 support in this whole project is a long story because of System.Drawing.
I believe that ref is not required in that case and removed it in this MR

@paulushub
Copy link
Contributor

Net8.0 support in this whole project is a long story because of System.Drawing.

Not tried it yet, but was thinking it should succeed with System.Drawing.Primitives, which is available as Nuget package.

@paulushub paulushub added the bug label Jan 11, 2024
@paulushub
Copy link
Contributor

@RomanKalachik Please can you share the issues you have with the .NET 8 support?
Currently, the support for System.Drawing is fixed at version 5.0.3 in the SVG-NET.
I downloaded your project, added the .NET 8, it builds without any issue.
image

I tested all on command-lines:

dotnet test --framework net8.0 Tests/Svg.UnitTests/Svg.UnitTests.csproj
dotnet test --framework net6.0 Tests/Svg.UnitTests/Svg.UnitTests.csproj
dotnet test --framework netcoreapp3.1 Tests/Svg.UnitTests/Svg.UnitTests.csproj
dotnet test --framework net462 Tests/Svg.UnitTests/Svg.UnitTests.csproj

All tests succeeded except 1, and it is due to difference in line ending. Still do not know why this fails with your source code (I downloaded it, not a git clone).
Untitled1

@paulushub paulushub self-assigned this Jan 16, 2024
@paulushub
Copy link
Contributor

paulushub commented Jan 16, 2024

Why does your PR excludes cases as shown below for the ReadOnlySpan<char> (the CoordinateParserState is modified so ref is expected).

Svg.Helpers.StringParser

	public static float ToFloat(ref ReadOnlySpan<char> value)
	{
#if NETSTANDARD2_1 || NETCOREAPP2_1_OR_GREATER
		return float.Parse(value, NumberStyles.Float, Format);
#else
		return float.Parse(value.ToString(), NumberStyles.Float, Format);
#endif
	}

Svg.CoordinateParser

public static bool TryGetFloat(out float result, ref ReadOnlySpan<char> chars, ref CoordinateParserState state)
{
	var charsLength = chars.Length;

	while (state.CharsPosition < charsLength && state.HasMore)
	{
		var currentChar = chars[state.CharsPosition];

		// NOTE: Deleted for simplicity
	}
}

Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

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

Can you please add an entry in the release notes?

@paulushub paulushub requested review from paulushub and mrbean-bremen and removed request for paulushub and mrbean-bremen January 23, 2024 11:25
@paulushub
Copy link
Contributor

Issues raised here are fixed in #1137

@paulushub paulushub closed this Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants