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

Fix build break #99328

Closed
wants to merge 1 commit into from
Closed

Fix build break #99328

wants to merge 1 commit into from

Conversation

kg
Copy link
Contributor

@kg kg commented Mar 5, 2024

Draft PR because I'm not convinced this is the right fix.

@ghost
Copy link

ghost commented Mar 5, 2024

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: kg
Assignees: kg
Labels:

area-System.Text.Json

Milestone: -

@@ -398,7 +398,7 @@ public static byte[] ReaderLoop(int inpuDataLength, out int length, ref Utf8Json
ReadOnlySpan<byte> valueSpan = json.HasValueSequence ? json.ValueSequence.ToArray() : json.ValueSpan;
if (json.HasValueSequence)
{
Assert.True(json.ValueSpan == default);
Assert.True(json.ValueSpan.IsEmpty);
Copy link
Member

Choose a reason for hiding this comment

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

I think these tests wants to really test for default. This build break should be fixed by pragma warning disable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's my thinking too. I wanted to see if there are more breaks after these ones are fixed.

Copy link
Member

@buyaa-n buyaa-n Mar 5, 2024

Choose a reason for hiding this comment

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

Ah, the analyzer should be disabled for test projects, could you please add below rows here:

# CA2261: Do not use ConfigureAwaitOptions.SuppressThrowing with Task<TResult>
dotnet_diagnostic.CA2261.severity = none

# CA2265: Do not compare Span<T> to 'null' or 'default'
dotnet_diagnostic.CA2265.severity = none

Copy link
Member

Choose a reason for hiding this comment

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

I see the analyzer updated reverted, this can be added later when analyzer updated

@jkotas jkotas requested a review from buyaa-n March 5, 2024 20:16
@vcsjones
Copy link
Member

vcsjones commented Mar 5, 2024

There are still more build issues this PR does not address, like in src/libraries/System.Runtime/tests/System.Runtime.Tests/:

/Users/vcsjones/Projects/runtime/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/StringTests.cs(1085,25): error CA2265: Comparing a span to 'default' might not do what you intended, make the code more explicit by checking 'IsEmpty' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2265) [/Users/vcsjones/Projects/runtime/src/libraries/System.Runtime/tests/System.Runtime.Tests/System.Runtime.Tests.csproj::TargetFramework=net9.0-unix]

@kg kg closed this Mar 21, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants