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

ore: Add must_use to drain_filter_swapping #27315

Merged
merged 1 commit into from
May 27, 2024

Conversation

jkosh44
Copy link
Contributor

@jkosh44 jkosh44 commented May 27, 2024

This commit adds the #[must_use] annotation to the drain_filter_swapping function. drain_filter_swapping is evaluated lazily, so if the result is not used then it has no effect, and it is almost certainly a mistake. The annotation helps avoid this mistake by emitting a compiler warning.

Motivation

This PR refactors existing code.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • This PR includes the following user-facing behavior changes:
    • There are no user-facing behavior changes.

This commit adds the `#[must_use]` annotation to the
`drain_filter_swapping` function. `drain_filter_swapping` is evaluated
lazily, so if the result is not used then it has no effect, and it is
almost certainly a mistake. The annotation helps avoid this mistake by
emitting a compiler warning.
@jkosh44 jkosh44 requested a review from benesch as a code owner May 27, 2024 16:56
@jkosh44 jkosh44 enabled auto-merge (squash) May 27, 2024 16:57
Copy link
Contributor

@teskje teskje left a comment

Choose a reason for hiding this comment

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

Seems like a good hotfix to prevent bugs, but the documentation of this method is misleading too:

    /// Using this method is equivalent to the following code:
    ///
    /// ```
    /// # let some_predicate = |x: &mut i32| { *x == 2 || *x == 3 || *x == 6 };
    /// # let mut vec = vec![1, 2, 3, 4, 5, 6];
    /// let mut i = 0;
    /// while i < vec.len() {
    ///     if some_predicate(&mut vec[i]) {
    ///         let val = vec.swap_remove(i);
    ///         // your code here
    ///     } else {
    ///         i += 1;
    ///     }
    /// }
    ///
    /// # assert_eq!(vec, vec![1, 5, 4]);
    /// ```

This should instead say something like "using this method and consuming the iterator is equivalent to the following code:". Also it might be nice to have a little warning in the # Note section as well.

Copy link
Member

@ParkMyCar ParkMyCar left a comment

Choose a reason for hiding this comment

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

LGTM

You can also add a msg to the must_use annotation to explain why it must be used

@jkosh44 jkosh44 merged commit dbecc3c into MaterializeInc:main May 27, 2024
76 checks passed
@jkosh44 jkosh44 deleted the drain-filter-swapping-must-use branch May 28, 2024 14:12
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.

3 participants