tyencode: Make sure that projection bounds are handled in stable order. #34805
Conversation
|
cc @eddyb |
|
@bors r+ |
|
📌 Commit 5ad5072 has been approved by |
|
triage: beta-nominated |
| .iter() | ||
| .map(|b| (b.item_name().as_str(), b)) | ||
| .collect(); | ||
| projection_bounds.sort_by_key(|&(ref name, _)| name.clone()); |
There was a problem hiding this comment.
Newbie question (and total nit): why couldn't this basically be the following (or something close to it)?
let mut projection_bounds = bs.projection_bounds
.clone()
.sort_by_key(|&b| b.item_name().as_str());Because then it seems we'd be able to keep the next line just for tp in &projection_bounds { ... }, avoiding the extra call to map.
And even if we didn't do this the next line still seems like it could stay for tp in &projection_bounds with s/tp.0/tp.1.0 in the loop body.
There was a problem hiding this comment.
Since sort_by_key() returns nothing, projection_bounds with be the unit value (). It would have to look at least like:
let mut projection_bounds = bs.projection_bounds.clone();
projection_bounds.sort_by_key(|&b| b.item_name().as_str());And why didn't I do it like the above? In order to avoid that b.item_name().as_str() gets called more than once per item. Although, I somehow mistakenly assumed that b.item_name().as_str() would need to hash the string each time when looking it up in the interner, which it doesn't, so this "optimization" is even more premature than I thought :)
So, your version, when fixed as indicated, would actually be nicer than mine.
There was a problem hiding this comment.
Ahh, because sort_by_key would make basically O(n log n) calls to its closure, i.e. to b.item_name().as_str()? Makes much more sense now (notwithstanding the question of whether it's a premature optimization :). And yeah I overlooked the fact that sort_by_key returns unit, so your fix makes sense. Thanks for the explanation! Up to you what changes you want to make, if any.
|
@bors: p=1 (fixing a regression) |
tyencode: Make sure that projection bounds are handled in stable order. Fixes #34796. r? @alexcrichton
Backport rust-lang#34805 to beta
Fixes #34796.
r? @alexcrichton