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

Issues with Some.cs #13

Closed
tejacques opened this issue Dec 10, 2014 · 2 comments
Closed

Issues with Some.cs #13

tejacques opened this issue Dec 10, 2014 · 2 comments

Comments

@tejacques
Copy link

I think two changes should be made to Some.cs

First change: The overrides which call CheckInitialized should call their methods on the return value of CheckInitialized, rather than the value they pass in:

public override string ToString() =>
    CheckInitialised(Value.ToString());

public override int GetHashCode() =>
    CheckInitialised(Value.GetHashCode());

public override bool Equals(object obj) =>
    CheckInitialised(Value.Equals(obj));

Should be

public override string ToString() =>
    CheckInitialised(Value).ToString();

public override int GetHashCode() =>
    CheckInitialised(Value).GetHashCode();

public override bool Equals(object obj) =>
    CheckInitialised(Value).Equals(obj);

Reason:

If Value is not initialized it could have another exception thrown (such as NPE) because it's calling a method on Value before checking whether Value is initialized.

Second change: It's not enough to only check (and throw) in the default constructor because the CLR will not call the default constructor on arrays of structs.

Example:

var someThings = new Some<object>[100];
Console.WriteLine(someThings[0].Value == null); // Prints true, no exception raised

Reason:

For performance reasons, the CLR will not call the default constructor on struct arrays. It will only zero out the memory associated with them. You'll have to raise an error in the getter property to catch this case.

louthy added a commit that referenced this issue Dec 11, 2014
 - Some silly mistakes slipped through the net with checkin the Value property of Some<T>
 - Thanks to @tejacques for pointing them out!

#13
@louthy
Copy link
Owner

louthy commented Dec 11, 2014

First change: The overrides which call CheckInitialized ...

Yep, some silly mistakes there. I've taken a different approach to fixing it:

        public T Value => 
            CheckInitialised(value);

        public static implicit operator T(Some<T> value) => 
            value.Value;

        public override string ToString() =>
            Value.ToString();

        public override int GetHashCode() =>
            Value.GetHashCode();

        public override bool Equals(object obj) =>
            Value.Equals(obj);

It puts slightly more burden on the Value access, but I think overall it's cleaner.

For performance reasons, the CLR will not call the default constructor on struct arrays.

It's not the CLR, it's the C# compiler. It could easily call the same series of op-codes that it does for classes. It is however (like you say) an optimisation. I believe the C# team are discussing default constructors for the final C# 6. So we'll see how that goes. Until then Some<T> will always be a weak solution to this problem.

I do discuss this on the project home page, and explain its limitations. I think only using Some<T> for method arguments, and never instantiating them manually is probably the best use of a weak solution.

Thanks for the heads up on the bugs!

@tejacques
Copy link
Author

No problem -- I like the way you handled everything, I think that makes it work out nicely. Closing this issue now.

StefanBertels pushed a commit to StefanBertels/language-ext that referenced this issue Aug 13, 2018
format Exception to display full details
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

No branches or pull requests

2 participants