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 Nullalble to throw logic errors instead of exceptions #1103

Merged
merged 1 commit into from Feb 10, 2013
Merged

Fix Nullalble to throw logic errors instead of exceptions #1103

merged 1 commit into from Feb 10, 2013

Conversation

monarchdodra
Copy link
Collaborator

See also:
http://d.puremagic.com/issues/show_bug.cgi?id=9404

Basically, Nullable[Flavor] was not self assignable. Attempts to do self assignement would compile, because it extracts the value value via get, and then uses opAssign(T value). This can throw an exception if the other nullable is null though.

The problem with this is that it makes this(this) andopAssign` inconsistent (postblit never throws). This makes things particularly tricky if you start nesting a Nullable inside a struct:

struct S1
{
    Nullable!int ni;
    //default S1 s1 = S1(myNullable); works
    //default s1 = s2; works
}

struct S2
{
    Nullable!int ni;
    this(Nullable!int n)
    {ni = n;} //Throws
    void opAssign(S2 other)
    {ni = other.ni;} //Throws
}

This pull simply give all Nullables the function void opAssign()(typeof(this) other). This makes the current nullable a copy of other.

Also in this pull:

  • Remove 4424 workaround
  • Give failed enforcements error messages
  • Workaround for 9416
  • Extra unittests

{
.destroy(_value);
_isNull = true;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While I realize the if is not strictly needed, and that objects can be legally destroyed multiple times (which is what happens anyways when the nullable itself is destroyed), I figure we can still avoid doing so when possible.

@monarchdodra
Copy link
Collaborator Author

I also have a question/comment, that's related (though won't change the current pull):

Nullable.get will throw an exception if you read it while it is null. Afaik, a null read has always been an error.

Making an exception means

  • Code can't be nothrow
  • Extra costs when you know your nullable isn't null anyways.

Any chance we can make "get" throw errors? Or is that a breaking change?

@alexrp
Copy link
Member

alexrp commented Jan 29, 2013

It would be a breaking change, but I highly doubt anyone actually would be affected by it. Getting the value of a null nullable is a logic error for sure.

@monarchdodra
Copy link
Collaborator Author

OK. Thanks. I'll save the change for a subsequent pull.

@monarchdodra
Copy link
Collaborator Author

Given that Kenji correct the initial 9404 problem, my fix is not needed anymore.

I'm preserving the unittests and improvements though, and doing the enforce => assert change.

alexrp added a commit that referenced this pull request Feb 10, 2013
Fix Nullalble to throw logic errors instead of exceptions
@alexrp alexrp merged commit b314344 into dlang:master Feb 10, 2013
@monarchdodra monarchdodra deleted the Nullable branch March 11, 2013 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants