Skip to content

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
Contributor

You've got style failures with Travis.

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
Contributor

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
Contributor

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
Contributor

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
Contributor

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
Contributor

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
Contributor

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

@JackStouffer
Copy link
Contributor

Merging is blocked, it needs to be approved

@andralex
Copy link
Member Author

@burner will work on the changelog. Thanks!

@wilzbach
Copy link
Contributor

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.