Skip to content

Fix Issue 9183 - Add a Nullable.get(x) overload#5695

Merged
dlang-bot merged 1 commit intodlang:masterfrom
RazvanN7:Issue_9183
Aug 22, 2017
Merged

Fix Issue 9183 - Add a Nullable.get(x) overload#5695
dlang-bot merged 1 commit intodlang:masterfrom
RazvanN7:Issue_9183

Conversation

@RazvanN7
Copy link
Collaborator

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close Bugzilla Description
9183 Add a Nullable.get(x) overload

std/typecons.d Outdated
/// ditto
@property get(T)(T datum) @safe pure nothrow
{
if (isNull)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: return isNull ? datum : _value;

std/typecons.d Outdated
}

/// ditto
@property get(T)(T datum) @safe pure nothrow
Copy link
Contributor

Choose a reason for hiding this comment

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

inout?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried, but I get a weird error and I don't understand exactly why :
std/typecons.d(2531): Error: pure function 'std.typecons.Nullable!(VariantN!32LU).Nullable.get' cannot call impure destructor 'std.variant.VariantN!32LU.VariantN.~this'

std/typecons.d Outdated
}

/// ditto
@property get(T datum) @safe pure nothrow
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be const as well?

You could get around the impure error by templating get and thus letting the compiler do the work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how's this?

Copy link
Contributor

@JackStouffer JackStouffer left a comment

Choose a reason for hiding this comment

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

Looks good, though I would change datum to default to be in line with other libraries/languages.

@RazvanN7
Copy link
Collaborator Author

@JackStouffer default is a keyword in D.

@dnadlinger
Copy link
Contributor

fallback would be less confusing and would actually describe what this does.

std/typecons.d Outdated
}

/// ditto
@property get(T)(T datum) inout @safe pure nothrow
Copy link
Member

Choose a reason for hiding this comment

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

You need to mark datum as inout as well, or else the return will always be const (because of the inferred type of the ?: expression).

Copy link
Member

Choose a reason for hiding this comment

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

Can you change the T to U as it masks the overarching template type from the Nullable struct?

@MetaLang
Copy link
Member

My only criticism is that the function name is only passingly related to what it actually does. I Would suggest something more descriptive like getOrElse or getWithDefault; something that signals that datum is a default or fallback value.

@JackStouffer
Copy link
Contributor

@MetaLang I think that problem is partially addressed by this being consistent with the same type of function in Python (and I think Ruby).

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.

8 participants