Simplify iterator logic for Fuse #24233
Conversation
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
|
It doesn't simplify, it just writes the same thing in a shorter way. Does it matter that we use an extra closure here that the optimizier needs to re-inline? Because of the likely code size increase (in code given to llvm), I think this is a net negative unfortunately. Sorry to be negative about your minor fix, I'm just trying to weigh the change's merits. |
I think it is cleaner and more idiomatic. I am utilizing
Compare the LLVM IR for these two examples: They're identical as far as I know, and one of them uses a closure, so I'm not convinced code will increase. |
|
As scott mentioned on IRC, a better rewrite would be: let x = self.iter.next();
if x.is_none() {
self.done = true;
}
xas that makes the intention clear. |
frewsxcv
force-pushed
the
cleanup-fuse-iterator
branch
from
bebd57d to
b4cbeda
Compare
|
@lfairy Good idea, just force pushed with that change |
| None | ||
| } | ||
| x => x | ||
| let next = self.iter.next(); |
frewsxcv
force-pushed
the
cleanup-fuse-iterator
branch
from
b4cbeda to
a5c8e41
Compare
| x => x | ||
| let next = self.iter.next_back(); | ||
| if next.is_none() { | ||
| self.done = true; |
There was a problem hiding this comment.
Hmm, what about just self.done = next.is_none()? No branch at all, but unconditional mutation.
There was a problem hiding this comment.
I think writing self.done = true makes explicit the transition from false -> true.
But I'm not too attached to the idea.
There was a problem hiding this comment.
I was about to say the same thing as @lfairy , but I too don't feel too strongly about it, especially considering how small of a change this commit is
There was a problem hiding this comment.
Anytime I see a done variable I instantly go to "this is a one-transition boolean", personally.
There was a problem hiding this comment.
Force pushed with that change
frewsxcv
force-pushed
the
cleanup-fuse-iterator
branch
from
a5c8e41 to
5c80b7a
Compare
|
@bors r+ rollup |
|
📌 Commit 5c80b7a has been approved by |
|
If the IR is identical, it's because it's after optimization, I was thinking of the size of llvm's input from rustc from the beginning. Now with the closure gone, the point is moot and that's well. |
|
I think this change breaks the fuse() impl. Before done would only be set to true if next() returned None. Now it would be set to false again if it isn't. You want the rewrite @lfairy posted with the self.done inside the if. |
@cristicbz Can you give me an example where it would be set to |
|
Never mind, sorry, I didn't read it properly---I missed the big if self.done. |
|
⌛ Testing commit 5c80b7a with merge c3f77c7... |
|
⛄ The build was interrupted to prioritize another pull request. |
No description provided.