Skip to content

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

Closed
thestinger wants to merge 1 commit into
rust-lang:masterfrom
thestinger:times
Closed

(RFC) remove the Times trait in favour of range#8371
thestinger wants to merge 1 commit into
rust-lang:masterfrom
thestinger:times

Conversation

@thestinger

Copy link
Copy Markdown
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

graydon commented Aug 7, 2013

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

bstrie commented Aug 7, 2013

Copy link
Copy Markdown
Contributor

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

brson commented Aug 7, 2013

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

brson commented Aug 8, 2013

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

brson commented Aug 18, 2013

Copy link
Copy Markdown
Contributor

I want to solicit other opinions.

@brson

brson commented Aug 18, 2013

Copy link
Copy Markdown
Contributor

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

@thestinger

Copy link
Copy Markdown
Contributor Author

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

@glaebhoerl

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

U007D pushed a commit to U007D/rust-mos that referenced this pull request Aug 21, 2026
8371: Don't use HirDisplayWrapper when displaying SourceCode r=matklad a=Veykril

The issue was basically that when displaying for `DisplayTarget::SourceCode` some `hir_fmt` functions would create `HirDisplayWrapper`s which would then `fmt` these triggering the Display panic since `fmt::Display` can't fail the same way as `HirDisplay`. Simple fix is to just use `hir_fmt` directly. Should probably write that down somewhere in source, looking for a good spot to put that right now.

Fixes rust-lang#8077, Fixes rust-lang#8370


Co-authored-by: Lukas Wirth <lukastw97@gmail.com>
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