Skip to content

[tests] basic min/max test cases #566

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

Merged
merged 2 commits into from
Dec 16, 2015

Conversation

frootloops
Copy link
Contributor

No description provided.

@frootloops frootloops changed the title [test] basic min/max test cases [tests] basic min/max test cases Dec 15, 2015
@gribozavr
Copy link
Contributor

@frootloops Thank you! We have some basic tests in validation-test/stdlib/Algorithm.swift, could you consolidate these?

Also, could you rewrite the tests using MinimalComparableValue and check the identity of the object being returned? The identity will be arbitrary (not following any useful rule) now, but it is important to constrain it. If you feel like doing library code changes, you could implement what we discussed in pull request #137 that would make the identity useful:

On a preordered set, the argument goes, min of two equivalent elements should return the first parameter, and max should return the second.

@gribozavr gribozavr self-assigned this Dec 16, 2015
@frootloops
Copy link
Contributor Author

@gribozavr can you have a look through the PR. I rewrite max<T : Comparable>(x: T, _ y: T, _ z: T, _ rest: T...) -> T and I need your code review because I suspect it may be slower than the previous version.

Thank you for MinimalComparableValue it makes writing test more easier.

expectEqual(1, max(0, 0, 1))
}

Algorithm.test("Min/Max identity check") {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to have separate value and identity tests. All value tests could just check identity as well.

@frootloops frootloops force-pushed the test-algorithm branch 2 times, most recently from 8ce74e8 to 179600a Compare December 16, 2015 18:02
@frootloops
Copy link
Contributor Author

@gribozavr done

r = t
}
var r = x
for t in [y, z] + rest where t >= r {
Copy link
Contributor

Choose a reason for hiding this comment

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

You are still creating an array here, this will be slow.

@frootloops
Copy link
Contributor Author

@gribozavr what do you think about next code:

public func max<T : Comparable>(x: T, _ y: T, _ z: T, _ rest: T...) -> T {
  var r = max(max(x, y), z)
  for t in rest where t >= r {
    r = t
  }
  return r
}

or do you prefer a classic way:

public func max<T : Comparable>(x: T, _ y: T, _ z: T, _ rest: T...) -> T {
  var r = x
  if y >= r {
    r = y
  }
  if z >= r {
    r = z
  }
  for t in rest where t >= r {
    r = t
  }
  return r
}

@gribozavr
Copy link
Contributor

@frootloops As long as it would be possible to implement the idea discussed in #137, I'm ok with the short one.

@frootloops
Copy link
Contributor Author

@gribozavr please have a look. I hope it's all right.

@gribozavr
Copy link
Contributor

@frootloops Looks great, thanks! From the tests we see that we already implement #137.

gribozavr added a commit that referenced this pull request Dec 16, 2015
[tests] basic min/max test cases
@gribozavr gribozavr merged commit 4a7a496 into swiftlang:master Dec 16, 2015
@frootloops frootloops deleted the test-algorithm branch March 27, 2016 05:52
freak4pc pushed a commit to freak4pc/swift that referenced this pull request Sep 28, 2022
minor fixing: logical, and function issue lint
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.

2 participants