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

Deriving bug fixes #8285

Merged
merged 4 commits into from
Aug 7, 2013
Merged

Deriving bug fixes #8285

merged 4 commits into from
Aug 7, 2013

Conversation

huonw
Copy link
Member

@huonw huonw commented Aug 4, 2013

Some general clean-up relating to deriving:

  • TotalOrd was too eager, and evaluated the .cmp call for every field, even if it could short-circuit earlier.
  • the pointer types didn't have impls for TotalOrd or TotalEq.
  • the Makefiles didn't reach deep enough into libsyntax for dependencies.

(Split out from #8258.)

huonw added 3 commits August 4, 2013 19:37
Previously it would call:

  f(sf1.cmp(&of1), f(sf2.cmp(&of2), ...))

(where s/of1 = 'self/other field 1', and f was
std::cmp::lexical_ordering)

This meant that every .cmp subcall got evaluated when calling a derived
TotalOrd.cmp.

This corrects this to use

   let test = sf1.cmp(&of1);
   if test == Equal {
      let test = sf2.cmp(&of2);
      if test == Equal {
        // ...
      } else {
        test
      }
   } else {
     test
   }

This gives a lexical ordering by short-circuiting on the first comparison
that is not Equal.
@huonw huonw mentioned this pull request Aug 4, 2013
@alexcrichton
Copy link
Member

Could you add the tests into this PR as well (the ones that asserted short circuiting)

Other than that, r=me

@huonw
Copy link
Member Author

huonw commented Aug 5, 2013

Done.

bors added a commit that referenced this pull request Aug 7, 2013
Some general clean-up relating to deriving:
- `TotalOrd` was too eager, and evaluated the `.cmp` call for every field, even if it could short-circuit earlier.
- the pointer types didn't have impls for `TotalOrd` or `TotalEq`.
- the Makefiles didn't reach deep enough into libsyntax for dependencies.

(Split out from #8258.)
@bors bors closed this Aug 7, 2013
@bors bors merged commit 4f3944a into rust-lang:master Aug 7, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 17, 2022
…5, r=Manishearth

Erase late bound regions in `iter_not_returning_iterator`

fixes rust-lang#8285

changelog: None
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