btree: cleanup difference, intersection, is_subset by hkBst · Pull Request #147808 · rust-lang/rust

hkBst · 2025-10-17T12:12:18Z

No description provided.

rustbot · 2025-10-17T12:12:23Z

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

joboet

Yeah, this is much cleaner, thank you!

View changes since this review

joboet · 2025-11-01T15:57:30Z

Please omit the changes not related to alloc from this PR – if something breaks here, doing them in a batch makes bisecting really difficult.

joboet · 2025-11-01T16:15:59Z

+ match self_min.cmp(other_min) {
+ Less => return false,
+ Equal => self_iter.next(),
+ Greater => None,
 };
- let (other_min, other_max) =
- if let (Some(other_min), Some(other_max)) = (other.first(), other.last()) {
- (other_min, other_max)
- } else {
- return false; // other is empty
+ match self_max.cmp(other_max) {
+ Greater => return false,
+ Equal => self_iter.next_back(),
+ Less => None,
 };


Since these values are never used, I find the None arms very confusing, so I wouldn't change this. But a comment on the next calls explaining that they remove self_min/self_max from the iteration wouldn't hurt.

joboet · 2025-11-01T16:25:02Z

+ // skip over elements that are smaller
+ // happens up to `ITER_PERFORMANCE_TIPPING_SIZE_DIFF * self.len() - 1` times


I don't find the "happens n times" notes to be particularly helpful – it'd be better to have the comments explain why the action chosen is the right one.

The comments explain why this ordering is used (from most frequent to least frequent), but I'll see about adding some comments explaining correctness.

I've added those comments.

joboet · 2025-11-01T16:26:55Z

- Greater => (),
- }
- }
+ self.is_empty()


I think it'd be better to preserve the old control flow, but express it using let-else statements. I.e.:
let(Some(self_min),Some(self_max)) = (self.first(),self.last())else{// This set is empty, and the empty set is a subset of all sets.returntrue;};

Yeah, that could work too. I'll give it a try.

This is pretty nice actually! Good catch.

rustbot · 2025-11-04T15:37:46Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

hkBst · 2025-11-04T15:41:56Z

@joboet Thanks for your thorough review! I think I addressed all your concerns.

@rustbot ready

hkBst · 2025-11-04T15:43:27Z

@joboet BTW Are there any performance tests for BTree? I could not immediately find them...

joboet · 2025-11-06T10:17:46Z

@joboet BTW Are there any performance tests for BTree? I could not immediately find them...

There in library/alloctests/benches/btree.

joboet

This looks fine to me! I'd be interested in the benchmark results, but don't mind merging this as–is even without them – there aren't really any control-flow changes here.

View changes since this review

run-make tests: use edition 2024 Bump run-make tests to edition 2024 to prevent test failures when using 2024 idioms in included code, such as I ran into here: rust-lang#147808.

hkBst · 2025-11-06T13:19:44Z

From running benchmarks locally I believe there is no perf impact of these changes.

joboet · 2025-11-06T13:58:51Z

All right! Then
@bors r+

bors · 2025-11-06T13:58:55Z

📌 Commit acd0294 has been approved by joboet

It is now in the queue for this repository.

Rollup merge of #148528 - hkBst:run-make-tests-1, r=jieyouxu run-make tests: use edition 2024 Bump run-make tests to edition 2024 to prevent test failures when using 2024 idioms in included code, such as I ran into here: #147808.

Rollup of 12 pull requests Successful merges: - #145768 (Offload device) - #145992 (Stabilize `vec_deque_pop_if`) - #147416 (Early return if span is from expansion so we dont get empty span and ice later on) - #147808 (btree: cleanup difference, intersection, is_subset) - #148520 (style: Use binary literals instead of hex literals in doctests for `highest_one` and `lowest_one`) - #148559 (Add typo suggestion for a misspelt Cargo environment variable) - #148567 (Fix incorrect precedence caused by range expression) - #148570 (Fix mismatched brackets in generated .dir-locals.el) - #148575 (fix dev guide link in rustc_query_system/dep_graph/README.md) - #148578 (core docs: add notes about availability of `Atomic*::from_mut_slice`) - #148603 (Backport 1.91.1 relnotes to main) - #148609 (Sync str::rsplit_once example with str::split_once) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #147808 - hkBst:btree-3, r=joboet btree: cleanup difference, intersection, is_subset

btree: cleanup difference, intersection, is_subset

rustbot assigned ibraheemdev Oct 17, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 17, 2025

This comment has been minimized.
Sign in to view

rust-cloud-vmsBotforce-pushed the btree-3 branch 2 times, most recently from 8e45e8f to 87755a7Compare October 17, 2025 13:18

This comment has been minimized.
Sign in to view

rust-cloud-vmsBotforce-pushed the btree-3 branch from 87755a7 to 31807b6Compare October 17, 2025 13:54

This comment has been minimized.
Sign in to view

rust-cloud-vmsBotforce-pushed the btree-3 branch from 31807b6 to c6181f6Compare October 17, 2025 15:15

rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Oct 17, 2025

This comment has been minimized.
Sign in to view

rust-cloud-vmsBotforce-pushed the btree-3 branch from c6181f6 to 5b3bd95Compare October 17, 2025 16:47

joboet requested changes Nov 1, 2025
View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 1, 2025

joboet assigned joboet and unassigned ibraheemdev Nov 1, 2025

rust-cloud-vmsBotforce-pushed the btree-3 branch from 5b3bd95 to 74b52ddCompare November 4, 2025 15:37

btree: cleanup difference, intersection, is_subset
acd0294

rust-cloud-vmsBotforce-pushed the btree-3 branch from 74b52dd to acd0294Compare November 4, 2025 15:38

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 4, 2025

hkBst mentioned this pull request Nov 6, 2025
run-make tests: use edition 2024 #148528
Merged

joboet approved these changes Nov 6, 2025
View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 6, 2025

jhpratt mentioned this pull request Nov 7, 2025
Rollup of 12 pull requests #148624
Merged

bors merged commit 4ad5a39 into rust-lang:masterNov 7, 2025
11 checks passed

rustbot added this to the 1.93.0 milestone Nov 7, 2025

rust-timer added a commit that referenced this pull request Nov 7, 2025
Unrolled build for#147808
5c3d327
Rollup merge of #147808 - hkBst:btree-3, r=joboet btree: cleanup difference, intersection, is_subset

hkBst deleted the btree-3 branch November 7, 2025 10:22

github-actionsBot pushed a commit to model-checking/verify-rust-std that referenced this pull request Nov 30, 2025
Rollup merge ofrust-lang#147808- hkBst:btree-3, r=joboet
59144c7
btree: cleanup difference, intersection, is_subset

		// skip over elements that are smaller
		// happens up to `ITER_PERFORMANCE_TIPPING_SIZE_DIFF * self.len() - 1` times

Uh oh!

Uh oh!

Conversation

hkBst commented Oct 17, 2025

Uh oh!

rustbot commented Oct 17, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

joboet left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Nov 4, 2025

Uh oh!

hkBst commented Nov 4, 2025

Uh oh!

hkBst commented Nov 4, 2025

Uh oh!

joboet commented Nov 6, 2025

Uh oh!

joboet left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hkBst commented Nov 6, 2025

Uh oh!

joboet commented Nov 6, 2025

Uh oh!

bors commented Nov 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

joboet left a comment •
edited by rustbot
Loading

joboet left a comment •
edited by rustbot
Loading