-
-
Notifications
You must be signed in to change notification settings - Fork 706
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
Restrict making of containers to non-infinite ranges #4860
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I think there are many more places in Phobos where we should be checking for isInfinite, such as std.array.array... I've been burned more than a couple times because I forgot that the range being passed to various consuming functions is actually an infinite range. I have one suggestion, which is to add another unit test testing the positive case where the range is not infinite. You can never have too many unit tests after all.
Edit: sorry, I'm still getting used to the new review system.
d914300 to
582ad72
Compare
| import std.container.array : Array; | ||
| import std.range : repeat; | ||
| import std.range.primitives : isInfinite; | ||
| static assert(!__traits(compiles, { auto arr = make!Array(repeat(5)); })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to have the positive case tested as well. Something like:
static assert(__traits(compiles, { auto arr = make!Array(only(5)); }));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
582ad72 to
ad5537d
Compare
|
Urgh, this is going to take some getting used to. |
|
I'm referring to the new Github review system. This is my first time using it and it's going to take some getting used to ;-) |
|
Aha. |
|
Two has approved...so why no merge? |
|
Auto-merge toggled on |
|
Thanks. |
|
@MetaLang |
|
Not sure, I'm guessing we'll just have to cross those bridges as we come to them. The only other functions I can think of at the moment are writeln et al. They'll happily take an infinite range and print out its elements forever. |
Seems like auto-tester has still now some troubles with automatically merging (it worked on a other PR), let's have an eye on this. I just deprecated one build which will hopefully trigger the merge soon. |
|
It's possible another PR merged. Github doesn't have great sync with auto tester, it sometimes keeps old status. |
Model-checking that forbids construction of containers from infinite generators.