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 #4407

Closed
wants to merge 1 commit into from
Closed

Conversation

tsbockman
Copy link
Contributor

@tsbockman tsbockman commented Jun 5, 2016

The purpose of this package is:

  1. To provide a high-level wrapper for the overflow detection intrinsics in core.checkedint, and
  2. To protect the user from various other undesirable/unintuitive behaviour of the built-in integer types, such as incorrect signed/unsigned comparisons.

For a more thorough explanation of what problems are solved, and how, please consult the package documentation.

I encourage reviewers to try out the DUB package, as it:

  1. Works on GDC and LDC, with far better performance than on DMD, and
  2. Contains a much more extensive test suite that should not be added to the normal Phobos unittest build, because it takes too much time (15+ minutes with DMD) and memory (5+ GB) to compile.

TODO: changelog entry

@tsbockman tsbockman force-pushed the checkedint branch 11 times, most recently from 433091b to 4d960b3 Compare June 5, 2016 13:13
@burner burner self-assigned this Jun 5, 2016
@burner
Copy link
Member

burner commented Jun 7, 2016

foreach (T; AliasSeq!(double, int[]))
assert(!(isBasicFixed!T || isFixedPoint!T));
}
+/
Copy link
Member

Choose a reason for hiding this comment

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

why is this in comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a checkedint-aware version of isFixedPoint, which is currently a private helper in checkedint's package.d, but which I would like to add to std.traits. Search package.d for:

// maybe add these to std.traits? ///////////////////////////

@JackStouffer
Copy link
Member

The table on the overview says int.max + 1 == 0 when it should be int.min.

@JackStouffer
Copy link
Member

I think checkedint.to should be renamed to something like safe/smartConvert, mainly to avoid confusion with conv.to, as they can have very different behaviors. It would also help promote the useful/different features of the two functions. This would require the conv.to forwarding to be removed.

@JackStouffer
Copy link
Member

Are isSafeInt and isSmartInt nessesary? Are they just aliases to isInstanceOf!(isSmartInt, T)?

@tsbockman
Copy link
Contributor Author

The table on the overview says int.max + 1 == 0 when it should be int.min.

Well that's embarassing (considering what I'm working on)! Thanks, and Fixed.

@tsbockman
Copy link
Contributor Author

tsbockman commented Jun 8, 2016

I think checkedint.to should be renamed to something like safe/smartConvert...

That seems reasonable (but the name should be shorter). I'll wait and see if anyone else has anything to say about that before actually changing it, though.

@JackStouffer
Copy link
Member

Overall the basic design/API looks solid, I haven't looked at the code yet though.

@tsbockman
Copy link
Contributor Author

Are isSafeInt and isSmartInt nessesary? Are they just aliases to isInstanceOf!(isSmartInt, T)?

One of my design goals for checkedint is to have complete, but readable template constraints and return types wherever possible.

isCheckedInt is much easier on the eyes than (isInstanceOf!(SmartInt, T) || isInstanceOf!(SafeInt, T)), and I think it would be weird to have isCheckedInt without isSmartInt and isSafeInt to go with it.

Regardless, it costs nothing to include them; it's not like we'd want to use those names for something else...

@tsbockman
Copy link
Contributor Author

tsbockman commented Jun 8, 2016

I don't suppose anyone can tell me why the SafeInt versus SmartInt table is duplicated in the DDOC version (but not the DDOX one)?

Even with DDOC, it's fine when I build the docs for my DUB package, which has nearly identical code.

@tsbockman tsbockman force-pushed the checkedint branch 3 times, most recently from bdc7f25 to 8054bd3 Compare June 15, 2016 06:27
@wilzbach
Copy link
Member

minor nitpick:

could you change the dmd links from

http://dlang.org/dmd-windows.html#switch-inline

to

$(ROOT_DIR)dmd.html#switch-inline

  • will work for subfolders or SSL
  • will redirect to the appropriate OS (linux, mac os x reference) (explanation)

Also is there a way to split up package.d? Github doesn't display it here due to the large size, which means we can't annotate it with comments :/

@tsbockman
Copy link
Contributor Author

@wilzbach

could you change the dmd links from
http://dlang.org/dmd-windows.html#switch-inline
to
$(ROOT_DIR)dmd.html#switch-inline

I changed it, but now the links don't work - that might just be because the $(ROOT_DIR) macro only works properly on the live website, though.

Could you take a look and let me know if I did it right?

Also is there a way to split up package.d? Github doesn't display it here due to the large size, which means we can't annotate it with comments :/

I'm not going to change the API layout just to work around a Github limitation that will become irrelevant as soon as this is accepted.

Just copy-paste a little of the context and stick it in a D code block when you want to comment; I'll be able to find the spot in the code easily enough.

@tsbockman
Copy link
Contributor Author

This has been rejected by andralex. So I'm going to close this unless anyone cares enough to go change his mind.

@tsbockman tsbockman closed this Jun 16, 2016
@JackStouffer
Copy link
Member

I hope you'll keep the dub package up with any of the fixes you made here.

@tsbockman
Copy link
Contributor Author

tsbockman commented Jun 16, 2016

@JackStouffer

I hope you'll keep the dub package up

Definitely.

I intend to support it, too - if any one actually uses it. Bug reports and pull requests are welcome.

I'll consider small feature requests as well, although I consider the design essentially complete.

@burner
Copy link
Member

burner commented Jun 16, 2016

@tsbockman I read the thread. I'm sorry it went this way. It seems the D Process itself need some bugfixes ;-)

anyway thank you for your effort. I hope you had some fun working on checkedint.

@tsbockman
Copy link
Contributor Author

@burner

It seems the D Process itself need some bugfixes ;-)

Simply telling people from the start whose approval they actually need would go a long way. I knew that andralex would be involved, but I had expected a community debate over the merits of the design, rather than an apparently abrupt and unilateral decision.

There are pros and cons to either approach, but it's not surprising that my effort here failed, given that it was designed to please the people who gave me feedback earlier in the process (and myself, of course), which did not include andralex.

anyway thank you for your effort.

You're welcome. Hopefully someone will find the DUB package useful, whether directly, or as a starting point for a rewrite.

I hope you had some fun working on checkedint.

I did indeed. :-)

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.

4 participants