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 Biginteger default. #1242

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Nov 13, 2024

This pr fix the BigInteger default value.

When we define a BigInteger, it will not be assigned a default value like int, but it will remain as null.

but when it is defined in a class/struct, the defualt constructor of of class/struct will assign it a default value.

@Hecate2
Copy link
Contributor

Hecate2 commented Nov 13, 2024

In what case do we utilize BigInteger default value as null? default(BigInteger) returns 0.

@Jim8y
Copy link
Contributor Author

Jim8y commented Nov 13, 2024

In what case do we utilize BigInteger default value as null? default(BigInteger) returns 0.

Well, i might not be clear, default value of BigInteger is 0, but it will not be assigned automatically when we define a BigInteger. I have tried, C# syntax analyer wont allow you to use an uninitialized BigInteger actually, it will show you an error.

image
Local variable 'a' might not be initialized before accessing

@Hecate2
Copy link
Contributor

Hecate2 commented Nov 13, 2024


I think enabling nullable is enough (in PR 1214)

@Jim8y
Copy link
Contributor Author

Jim8y commented Nov 13, 2024

I think enabling nullable is enough (in PR 1214)

I personally dont think we should enable nullable in contract, not really being much help to contract but introduces many complex work in compiler.... but since we already have it, so, lets complete 1214 then.

@Hecate2
Copy link
Contributor

Hecate2 commented Nov 13, 2024

I think enabling nullable is enough (in PR 1214)

I personally dont think we should enable nullable in contract, not really being much help to contract but introduces many complex work in compiler.... but since we already have it, so, lets complete 1214 then.

Nullable simply gives more hints to the user, and does not affect the compiler. If nullable is false, everything can be silently null, being more dangerous.

@Jim8y
Copy link
Contributor Author

Jim8y commented Nov 13, 2024

I think enabling nullable is enough (in PR 1214)

I personally dont think we should enable nullable in contract, not really being much help to contract but introduces many complex work in compiler.... but since we already have it, so, lets complete 1214 then.

Nullable simply gives more hints to the user, and does not affect the compiler. If nullable is false, everything can be silently null, being more dangerous.

Not in this pr, i mean compiler level nullable support that we already have, nullable value requires extra checks and increases the complexity.

@Hecate2
Copy link
Contributor

Hecate2 commented Nov 13, 2024

In this PR we are trying to mark BigInteger declared in method. However, if we apply this methodology, then for all types of variables declared in methods, they are all null, and all have to be checked with many efforts. Thus using in csproj brings more efficient and accurate checks, for free.
And for the potential drawback of making the compiler more complex, the complexity is actually not brought by a property in a csproj. It is always possible to write a ? after a type, no matter whether is enabled in the csproj.

{
// The default BigInteger is null.
// But object creation will assign BigInteger a default value
if (fields[i].Type.Name == nameof(BigInteger))
Copy link
Member

Choose a reason for hiding this comment

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

Its better to check types with namespace

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants