Skip to content

(RFC) remove the Times trait in favour of range #8371

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

Closed
wants to merge 1 commit into from
Closed

(RFC) remove the Times trait in favour of range #8371

wants to merge 1 commit into from

Conversation

thestinger
Copy link
Contributor

It's not possible to break from times and it results in the
restrictions coming from closures. Using for _ in range(0, n) from the
start is simpler, rather than switching to it after hitting either of
these issues.

@graydon
Copy link
Contributor

graydon commented Aug 7, 2013

I don't see how to special case the for loop syntax. It has to decide whether to parse the tokens after "for" as pattern or expr. How should it choose?

@thestinger
Copy link
Contributor Author

That's true, it's not as simple as I expected due to the need to avoid boundless lookahead. I guess that's the cost of putting the pattern first.

Whether or not we can make a short form, I still think we should be using for as the looping construct in these cases.

@bstrie
Copy link
Contributor

bstrie commented Aug 7, 2013

I've already registered my arguments against this in the comments on #8326. To reiterate the most important points:

  1. It's a method defined specifically for dead-simple repetition. If you're doing anything more clever than that, such as breaking, you're doing it wrong.
  2. As a simple method, it's designed to be an approachable example of do notation, higher-order functions, and defining methods on builtin types. It's tailor-made for tutorials and tiny code examples, which are Important For Marketing.
  3. It's more clear in its intent than for _ in range(0, x), especially if the anticipated ability to omit _ in is intractable. I still get friction every time that I do this in Python, out of fear of forgetting whether the upper bound is inclusive or exclusive.
  4. It's just damn useful!
$ git grep "do.*times" | wc -l
127
$ git grep "for.*range" | wc -l
256

Even before external iterators we'd had the ability to do for range(0, x) |_|, and people still preferred times.

  1. In total it's 12 lines of dead-simple code. No maintenance cost.

To repeat, I'm fine with removing all uses of times from the compiler if you think it will speed up build times by half a second. But removing the interface entirely is unwarranted.

@brson
Copy link
Contributor

brson commented Aug 7, 2013

This is a tough call for me. My gut tells me to not like .times, mostly just because of nebulous negative feelings about being clever and cute. But I do very often write things like for stress_factor().times { } - it doesn't get any more succinct.

With times no longer implementing for, do stress_factor().times { } sounds less cool than for.

If we do keep it, then I think it should stay in the prelude - as a convenience method it's much less convenient to require an import.

@thestinger
Copy link
Contributor Author

I find it less convenient, because you have no break, return or the ability to move out of values. It quickly leads to working around issues stemming from closures if you're new to the language.

An alternative is for x in 5.times { ... }, but the same caveat of it not being generic applies - it's an extra thing for every unsigned type to implement, in addition to the numeric traits.

@brson
Copy link
Contributor

brson commented Aug 8, 2013

I doubt that I've ever tried to break out of or return from .times. I mostly use it for stress testing concurrency where I just want to pound on threads for a while. From looking at the codebase I would guess that I'm the biggest user, and for that purpose.

@thestinger
Copy link
Contributor Author

The Times trait is missing implementations for u8, u16, u32 and u64 which is why it doesn't require the integer suffix. There's also no implementation for big unsigned integers.

Every single function/method/trait/type in the library is a burden, because it makes the language more complex. I don't think Times comes close to being worth a module, an implementation for every unsigned integer type and different a looping idiom. If a user does use Times and hits a closure capture error, it's not obvious that they can just switch to the regular looping mechanism since this isn't explained anywhere.

@thestinger thestinger closed this Aug 18, 2013
@thestinger thestinger deleted the times branch August 18, 2013 17:31
@thestinger thestinger restored the times branch August 18, 2013 17:33
@thestinger thestinger reopened this Aug 18, 2013
It's not possible to break from `times` and it results in the
restrictions coming from closures. Using `for _ in range(0, n)` from the
start is simpler, rather than switching to it after hitting either of
these issues.
@brson
Copy link
Contributor

brson commented Aug 18, 2013

I want to solicit other opinions.

@brson
Copy link
Contributor

brson commented Aug 18, 2013

Can Times be implemented generically the same way range is instead of redefining it for all integers?

@thestinger
Copy link
Contributor Author

If we made a trait for unsigned integers, it could be implemented from that.

@glaebhoerl
Copy link
Contributor

What about dropping the trait and just having times() be a method on uint? For the overwhelming minority of cases when you want a different type, there's for.

@catamorphism
Copy link
Contributor

Closing due to lack of activity. Feel free to open a new pull request if consensus gets reached on this!

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.

6 participants