Skip to content

Refactoring 1_stdlib/Optional.swift test file #423

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 3 commits into from
Dec 11, 2015

Conversation

frootloops
Copy link
Contributor

Can somebody have a look at the PR and leave some comments and advices about code style, variable naming etc.

I use ./llvm/utils/lit/lit.py -sv ./build/Ninja-DebugAssert/swift-macosx-x86_64/test-macosx-x86_64/1_stdlib/Optional.swift for running tests. Is it right way to do so?

protocol TestProtocol1 {}

// Check that the generic parameter is called 'Memory'.
extension Optional where Wrapped : TestProtocol1 {
extension Optional where Wrapped: TestProtocol1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the standard library code, we put spaces on both sides of the colon when it denotes an "is-a" relationship: in generic parameter lists, in inheritance and protocol implementation lists etc.

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

@gribozavr thanks for review. Can you look again :)

@frootloops
Copy link
Contributor Author

Whats about Optional<Int> vs Int? What do you prefer?

@gribozavr
Copy link
Contributor

Int?, where parser accepts it.

@frootloops
Copy link
Contributor Author

@gribozavr Optional<Int> and Int? are they equal?

@gribozavr
Copy link
Contributor

@frootloops Yes, T? is syntax sugar for Optional<T>.

@frootloops
Copy link
Contributor Author

@gribozavr ok, thanks.

@frootloops
Copy link
Contributor Author

@gribozavr is PR ready for merge?

// CHECK-NEXT: false, true, false, false, true, false.
expectEqual(testRelation(==), [true, false, false, false, false, true])
expectEqual(testRelation(!=), [false, true, true, true, true, false])
expectEqual(testRelation(<), [false, true, false, false, true, false])
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this into an OptionalTest.test() {}, there should be no top-level code.

@frootloops
Copy link
Contributor Author

@gribozavr I have update the PR. Thank you for comments 👍

gribozavr added a commit that referenced this pull request Dec 11, 2015
Refactoring 1_stdlib/Optional.swift test file
@gribozavr gribozavr merged commit 0cc3819 into swiftlang:master Dec 11, 2015
@gribozavr
Copy link
Contributor

@frootloops Thank you!

kateinoigakukun pushed a commit to kateinoigakukun/swift that referenced this pull request Mar 20, 2020
freak4pc pushed a commit to freak4pc/swift that referenced this pull request Sep 28, 2022
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