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

[stdlib] Used map and guard for nil handling #241

Merged
merged 1 commit into from
Dec 6, 2015

Conversation

codestergit
Copy link
Contributor

Changed nil handling by using map statements.
I think these changes should have guard let or map as it is more expressive.
Please let me know if anything else needed.
Thanks to swift team

@@ -177,9 +177,8 @@ public struct EnumerateGenerator<
///
/// - Requires: No preceding call to `self.next()` has returned `nil`.
public mutating func next() -> Element? {
let b = base.next()
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be implemented using Optional.map:

public mutating func next() -> Element? {
  return base.next().map { (index: count++, element: $0) }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

-1 from me on this one because of side effects inside map().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gribozavr Changed it to guard let. What side effect it cause?

Copy link
Contributor

Choose a reason for hiding this comment

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

The increment in index++ was side effect.

@codestergit
Copy link
Contributor Author

@NachoSoto Thinking about that, also find these changes in #193. I am making these changes. Thanks for suggestion.

@codestergit codestergit changed the title Changed nil checking to more expressive guard let statement [stdlib] Used map for nil handling Dec 5, 2015
@codestergit
Copy link
Contributor Author

Done changes to handle nil by map.

@codestergit
Copy link
Contributor Author

@gribozavr Thanks for reviewing this. I have done the changes. I have also done some performance testing and I get same results. All if/nil , map and guard are tanking almost same amount of time. You can find gist here
Please let me know if any other changes required.
I have run the test on apple swift master branch(this does not include my changes) and found some of test are failing on my machine. Here is output. Build is fine but do you know what stupid thing I have done while running the tests. I run same test with my changes and I got same response.
Thanks for your help

@codestergit codestergit changed the title [stdlib] Used map for nil handling [stdlib] Used map and guard for nil handling Dec 5, 2015
@gribozavr
Copy link
Contributor

@codestergit I ran the tests and couldn't reproduce the failure.

gribozavr added a commit that referenced this pull request Dec 6, 2015
[stdlib] Used map and guard for nil handling
@gribozavr gribozavr merged commit 8bba74e into swiftlang:master Dec 6, 2015
@codestergit
Copy link
Contributor Author

@gribozavr Thank for merging. Sorry but just for my knowledge want to know the index++ side effect. I am not that much good with higher order functions.How map will affect or behave different way. Optional.map has following implementation

public func map<U>(@noescape f: (Wrapped) throws -> U) rethrows -> U? {
    switch self {
    case .Some(let y):
      return .Some(try f(y))
    case .None:
      return .None //I think it will return as it is and f will not execute and `index++` should not happen
    }
  }

so it is also returning early if our base.next() return nil value or .None. It will not execute clousre and count value should remain same. I have tested this and both are giving same result. Is I missing something. Thanks for being so kind :)

@gribozavr
Copy link
Contributor

@codestergit It should behave correctly just as you expect, it is just poor style to use side-effecting operations with APIs influenced by the functional paradigm, like map().

@codestergit
Copy link
Contributor Author

Thanks @gribozavr .

dabelknap added a commit to dabelknap/swift that referenced this pull request May 7, 2019
maldahleh pushed a commit to maldahleh/swift that referenced this pull request Oct 26, 2020
…hanges

Update gyb generated files for _specialize syntax addition
freak4pc pushed a commit to freak4pc/swift that referenced this pull request Sep 28, 2022
Separate vapor project into subcomponents (SR-7233)
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