Skip to content
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

docs(slice): Clarification in binary_search_by #46870

Merged
merged 1 commit into from
Dec 20, 2017
Merged

docs(slice): Clarification in binary_search_by #46870

merged 1 commit into from
Dec 20, 2017

Conversation

ffflorian
Copy link
Contributor

@ffflorian ffflorian commented Dec 20, 2017

This PR fixes a small comment typo adds some clarification to a half-open interval in the binary_search_by function in slice.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @withoutboats (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@hanna-kruppe
Copy link
Contributor

Thanks for the PR!

However, I don't think this is a typo. [a, b) denotes a half-open interval (>= a and < b) as opposed to a closed interval [a, b] (>= a and <= b), and mid < size is exactly what is needed for this code to work correctly. But perhaps we could clarify the comment a bit to make this clearer for people who haven't seen that notation before?

@ffflorian
Copy link
Contributor Author

@rkruppe
Oh, I understand. Yes, I can add some clarification. Thanks!

@@ -400,14 +400,14 @@ impl<T> SliceExt for [T] {
while size > 1 {
let half = size / 2;
let mid = base + half;
// mid is always in [0, size).
// mid is always in [0, size].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a connective here, e.g., "mid is always in [0, size), i.e., [bullet points below]"

// mid >= 0: by definition
// mid < size: mid = size / 2 + size / 4 + size / 8 ...
let cmp = f(unsafe { s.get_unchecked(mid) });
base = if cmp == Greater { base } else { mid };
size -= half;
}
// base is always in [0, size) because base <= mid.
// base is always in [0, size] because base <= mid.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and here maybe spell out base < size?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was just thinking about how to make this line more explicit, too. But if you don't see the need to change it, that's fine by me too.

@ffflorian ffflorian changed the title fix(slice): Typo in binary_search_by fix(slice): Clarification in binary_search_by Dec 20, 2017
@ffflorian
Copy link
Contributor Author

ffflorian commented Dec 20, 2017

@rkruppe
Sorry, your change requests appeared only after I pushed my latest commit.

Copy link
Contributor

@hanna-kruppe hanna-kruppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look basically good. However, I'm afraid I'll have to ask you to squash the commits into one -- generally the project prefers to not have unfinished and "undo this" commits, but rather one commit per finished logical step.

Since you're touching the commits anyway, you might want to consider the subjective style issue below.

@@ -400,14 +400,15 @@ impl<T> SliceExt for [T] {
while size > 1 {
let half = size / 2;
let mid = base + half;
// mid is always in [0, size].
// mid is always in [0, size).
// (that means mid is >= 0 and < size)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor style nit, but since the lines below are effectively subordinate bullet points, I think it would look nicer if this was one line. Fix if you want, or don't.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Dec 20, 2017

No worries! I guess I should have bundled them together with my first comment. What you wrote independently is good too, these were just suggestions.

@ffflorian
Copy link
Contributor Author

ffflorian commented Dec 20, 2017

@rkruppe
I implemented your suggested changes and squashed my commits into one. 🙂

@ffflorian ffflorian changed the title fix(slice): Clarification in binary_search_by docs(slice): Clarification in binary_search_by Dec 20, 2017
// mid >= 0: by definition
// mid < size: mid = size / 2 + size / 4 + size / 8 ...
let cmp = f(unsafe { s.get_unchecked(mid) });
base = if cmp == Greater { base } else { mid };
size -= half;
}
// base is always in [0, size) because base <= mid.
// base is always in [0, size) because base <= mid and base < size.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, as written that isn't exactly clarifying. Let's just drop it — you're the one who was confused by this and evidently you didn't see the need to clarify this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the PR or this line?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line. The other change is great :)

@hanna-kruppe
Copy link
Contributor

Great. Thanks again for taking the time, and sorry for all the back and forth ❤️

@bors r+

@bors
Copy link
Contributor

bors commented Dec 20, 2017

📌 Commit f6ab79d has been approved by rkruppe

@ffflorian
Copy link
Contributor Author

@rkruppe

sorry for all the back and forth

No worries, thanks for taking your time! 🙂

kennytm added a commit to kennytm/rust that referenced this pull request Dec 20, 2017
docs(slice): Clarification in binary_search_by

This PR ~fixes a small comment typo~ adds some clarification to a half-open interval in the `binary_search_by` function in `slice`.
@kennytm kennytm added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 20, 2017
bors added a commit that referenced this pull request Dec 20, 2017
Rollup of 14 pull requests

- Successful merges: #46359, #46517, #46671, #46751, #46760, #46787, #46794, #46828, #46831, #46835, #46851, #46852, #46856, #46870
- Failed merges:
@bors bors merged commit f6ab79d into rust-lang:master Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants