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

add std.experimental.checkedint #4613

Merged
merged 26 commits into from
Feb 24, 2017
Merged

add std.experimental.checkedint #4613

merged 26 commits into from
Feb 24, 2017

Conversation

andralex
Copy link
Member

This is the checkedint design I was discussing recently in the forum. It's work in progress - I think a couple more policies would be useful.

@thewilsonator
Copy link
Contributor

Line 177 you have a typo in the comment. ~= should be !=

@andralex
Copy link
Member Author

@thewilsonator fixed, thx!

@JackStouffer
Copy link
Member

You've got style failures with Travis.

posix.mak Outdated
@@ -170,7 +170,7 @@ PACKAGE_std = array ascii base64 bigint bitmanip compiler complex concurrency \
outbuffer parallelism path process random signals socket socketstream stdint \
stdio stdiobase stream string system traits typecons typetuple uni \
uri utf uuid variant xml zip zlib
PACKAGE_std_experimental = typecons
PACKAGE_std_experimental = checkedint typecons
Copy link
Member

Choose a reason for hiding this comment

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

it's annoying and we should really get rid off these two additional files, but until we do you need to update win32.mak & win64.mak too

Btw see this thread for the current status.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was glad I don't need to modify other places as well :o). Anyhow I made the changes to winNN.mak as well.

@JackStouffer
Copy link
Member

JackStouffer commented Jul 18, 2016

Could use some module level examples as well to help with the introduction.

struct Checked(T, Hook = Croak)
if (isIntegral!T && is(T == Unqual!T) || is(T == Checked!(U, H), U, H))
{
import std.algorithm : among;
Copy link
Member

Choose a reason for hiding this comment

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

import std.algorithm.comparison : among;

Copy link
Member Author

Choose a reason for hiding this comment

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

noice

@JackStouffer
Copy link
Member

payload appears in the docs and the unit tests despite being private.

{
alias Result = typeof(lhs + rhs);
import core.checkedint;
import std.algorithm : among;
Copy link
Member

Choose a reason for hiding this comment

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

import std.algorithm.comparison : among;

Copy link
Member Author

Choose a reason for hiding this comment

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

noice

@tsbockman
Copy link
Contributor

Checked needs some toString() overloads. Hook needs a hookToString function to handle NaN states.

@burner
Copy link
Member

burner commented Jul 19, 2016

Where is the dub package?

@tsbockman
Copy link
Contributor

The handling of return types is inconsistent:

1) Is the Hook supposed to be able to set the return type for each operation, or not? This would be required (along with some other changes) for a SmartInt-style type.

2) When a Checked type is expected, can a different Hook be set? This is required for manual compile-time range propagation (which someone requested as a feature).

3) Is it permitted to return some other type where a Checked instantiation would normally be expected? I don't think this is needed, but don't see much point to blocking it, either, since you're trying to make a flexible design. One possible use would be returning a floating-point value from an operation that is expected to overflow often, like exponentiation.

Whatever restrictions are placed on the return types should be documented somewhere - preferably in the function signature, instead of using auto.

@burner
Copy link
Member

burner commented Feb 24, 2017

Auto-merge toggled on

@burner
Copy link
Member

burner commented Feb 24, 2017

@andralex could you please create a PR for the changelog, so checkedint appears in the next release notes.

@wilzbach
Copy link
Member

How about adding a changelog file that shows-off a couple of features to the reader?

@JackStouffer
Copy link
Member

Merging is blocked, it needs to be approved

@andralex
Copy link
Member Author

@burner will work on the changelog. Thanks!

@wilzbach
Copy link
Member

2 of 4 checks passed

... -> #5188

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

Successfully merging this pull request may close these issues.