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

Modified min and max for code uniformity #188

Closed
wants to merge 2 commits into from
Closed

Modified min and max for code uniformity #188

wants to merge 2 commits into from

Conversation

qoobi
Copy link

@qoobi qoobi commented Dec 4, 2015

No description provided.

@gribozavr
Copy link
Contributor

Does this change implement what was discussed in pull request #137?

@colinta
Copy link

colinta commented Dec 4, 2015

This changes the behavior of max. If a class implements Comparable, calling max(a, b, c) currently returns the last instance that has the maximum value. This means that if a > b, b == c, the current version of max returns c, but your version returns b.

Neither is correct or wrong, but the change is unnecessary.

@qoobi
Copy link
Author

qoobi commented Dec 4, 2015

Also, currently max(a, b, c) returns b assuming a < b, b == c and max(a, b, c, d) returns d assuming a < b, b == c == d, which seems to be wrong. Following the argument that given equal parameters, min and max should return different instances, max should then return the last maximum instance while min returns the first minimum. I suggest the following code.

/// Returns the greatest argument passed.
@warn_unused_result
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 {
    if t >= r {
      r = t
    }
  }
  return r
}

@gribozavr
Copy link
Contributor

@mmkg Please update the pull request and provide tests.

@IngCr3at1on
Copy link

@colinta I think the general idea was to create more consistency in the language handling in the first place (both are "valid" but currently they are inconsistent to my understanding). This was discussed in great detail on #137

@gribozavr
Copy link
Contributor

#566 adds tests that show that we already implement #137.

@gribozavr gribozavr closed this Dec 16, 2015
dabelknap added a commit to dabelknap/swift that referenced this pull request Jan 16, 2019
…member-types

Ensure a space inside braces in a single member type decl.
maldahleh pushed a commit to maldahleh/swift that referenced this pull request Oct 26, 2020
[AutoDiff upstream] Update gyb-generated files.
freak4pc pushed a commit to freak4pc/swift that referenced this pull request Sep 28, 2022
XFAIL CoreStore for 3.2, 4.0 on {tv, watch}OS due to SDK changes
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.

4 participants