-
Notifications
You must be signed in to change notification settings - Fork 1k
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
.NET 9 #3576
base: master
Are you sure you want to change the base?
.NET 9 #3576
Conversation
@@ -54,7 +54,7 @@ public void TestGetBitLength() | |||
var random = new Random(); | |||
|
|||
// Big Number (net standard didn't work) | |||
VerifyGetBitLength(BigInteger.One << 32 << int.MaxValue, 2147483680); | |||
Assert.ThrowsException<OverflowException>(() => VerifyGetBitLength(BigInteger.One << 32 << int.MaxValue, 2147483680)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this change? the behavor is no longer the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BigInteger.One << 32 << int.MaxValue
will throw OverflowException in .net 9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this test is useless
@@ -25,7 +25,7 @@ public class UT_ByteExtensions | |||
public void TestToHexString() | |||
{ | |||
byte[] nullStr = null; | |||
Assert.ThrowsException<NullReferenceException>(() => nullStr.ToHexString()); | |||
Assert.ThrowsException<ArgumentNullException>(() => nullStr.ToHexString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean Net9.0 has changed the exception type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean Net9.0 has changed the exception type?
ToHexString
uses Convert.ToHexStrongLower
when run with NET.9.
May need to add an argument null-check to keep same semantics.
I rather not go to Also you missed |
I didn't miss global.json {
"sdk": {
"version": "9.0.100",
"rollForward": "latestFeature",
"allowPrerelease": false
}
} I’m not sure whether to upgrade to .NET 9. I'll leave it up to you core developers to decide! |
I am in favor, next year we move to long term again and so on. |
me too |
This will break the functionally of |
No dont leave it up to them. If it's dotnet 9.0 then have to upgrade. there is no way around it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you changing *.cs
files?
Because some of the exception types change, and some of the dotnet format rules change. |
That's true |
I would like to reduce the changes before merge it, and test this changes with the proper ut |
start += BitConverter.ToUInt32(buffer, 0); | ||
fs.Seek(sizeof(uint), SeekOrigin.Begin); | ||
} | ||
else | ||
{ | ||
fs.Read(buffer, 0, buffer.Length); | ||
fs.ReadExactly(buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the only place where code is changed in the core. @shargon , basically all rest are version number updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph was automatically changed after running dotnet format
in dotnet 9, and I saw that the logic hadn't changed, so I submitted it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
turn it off in the .editorconfig
}); | ||
|
||
ReadOnlySpan<byte> span = []; | ||
script.EmitPush(span); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried with data == null or data.IsEmpty, and this test doesn't work, and it should work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the warning shows Comparing a span to 'null' might be redundant, the 'null' literal will be implicitly converted to a 'Span<T>.Empty'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it worked like this in net8, we should remove the null check and push always empty array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But previously seems that with null we receive an error and with empty an empty push, not we can't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested new UT on master, it does not work as well
Test method Neo.Test.UT_ScriptBuilder.TestNullAndEmpty threw exception:
System.ArgumentNullException: Value cannot be null. (Parameter 'data')
at Neo.VM.ScriptBuilder.EmitPush(ReadOnlySpan`1 data) in C:\Users\liaoj\git\neo\src\Neo.VM\ScriptBuilder.cs:line 130
at Neo.Test.UT_ScriptBuilder.TestNullAndEmpty() in C:\Users\liaoj\git\neo\tests\Neo.VM.Tests\UT_ScriptBuilder.cs:line 57
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we should push always empty array and avoid the exception
update to .net 9