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] 0-ary tuples should be equatable #8354

Merged
merged 5 commits into from Jul 12, 2017
Merged

[stdlib] 0-ary tuples should be equatable #8354

merged 5 commits into from Jul 12, 2017

Conversation

@anayini
Copy link
Contributor

@anayini anayini commented Mar 26, 2017

Add an == method for comparing two empty tuples.

Resolves: SR-4172 (https://bugs.swift.org/browse/SR-4172)

Arjun Nayini
Add an == method for comparing two empty tuples.

Resolves: SR-4172 (https://bugs.swift.org/browse/SR-4172)
@anayini
Copy link
Contributor Author

@anayini anayini commented Mar 26, 2017

@swift-ci Please smoke test

Copy link
Collaborator

@CodaFi CodaFi left a comment

May as well do the full monty and edit the gyb template to make tuples quasi-Comparable too. This is also missing !=.

@CodaFi CodaFi requested a review from airspeedswift Mar 26, 2017
@anayini
Copy link
Contributor Author

@anayini anayini commented Mar 26, 2017

@CodaFi - Thanks. Just to clarify edit the .gyb to add !=, <, <=, >, >=? Does the bug need to be edited to include that?

@CodaFi
Copy link
Collaborator

@CodaFi CodaFi commented Mar 26, 2017

You should edit the GYB template to account for the non-generic 0-ary tuple. It should be possible to do this "in GYB's terms" so we don't litter this file with just this specific conformance.

@anayini
Copy link
Contributor Author

@anayini anayini commented Mar 26, 2017

Gotcha - was considering that adding extra logic for the 0 case in the existing templated methods might actually be less readable / more complicated (both the impls and comment generation will have to fork based on arity) than having a specific extra section for 0 outside of that, but can do that if preferred since it would litter less.

@practicalswift
Copy link
Collaborator

@practicalswift practicalswift commented Mar 27, 2017

👍 for the initiative/idea!

@jrose-apple jrose-apple requested a review from moiseev Mar 27, 2017
@moiseev
Copy link
Contributor

@moiseev moiseev commented Mar 27, 2017

Thanks @anayini! I agree that trying to incorporate the () into an existing gyb is not worth the effort, especially given that the one-tuple case will still be missing.
Please add the missing != and Comparable operators with doc comments.

@@ -23,6 +23,14 @@ comparableOperators = [

}%

/// Two tuples each of arity 0 are equal

This comment has been minimized.

@moiseev

moiseev Mar 27, 2017
Contributor

I would copy the heading from the 'generic' == on tuples, and later add a sentence that all empty-tuples are equal to each other as a note.

/// Returns a Boolean value indicating whether the corresponding components of
/// two tuples are equal.
///
/// All zero-arity tuples are equal.

Similarly for the < etc...

This comment has been minimized.

@anayini

anayini Mar 27, 2017
Author Contributor

👍

@anayini
Copy link
Contributor Author

@anayini anayini commented Mar 28, 2017

@moiseev - Updated to address your comments. Could also roll up the Comparable operators for the () into gyb format. Would probably separate out the comparableOperators list into two separate lists (one with the strict operators, one with the others) so that the template code could return true or false based on if they are unstrict operators or not for the () case.

/// Returns a Boolean value indicating whether any corresponding components of
/// the two tuples are not equal.
///
/// Two tuples each of arity zero are never unequal

This comment has been minimized.

@moiseev

moiseev Mar 28, 2017
Contributor

'never unequal' is a bit hard to understand (and there is a period missing ;-)). I'd go with the same note as for the ==.

This comment has been minimized.

@anayini

anayini Mar 28, 2017
Author Contributor

Makes sense

@moiseev
Copy link
Contributor

@moiseev moiseev commented Mar 28, 2017

I don't mind the non-gybbed version. @CodaFi , @airspeedswift what do you think?

Arjun Nayini
@CodaFi
CodaFi approved these changes Mar 28, 2017
@CodaFi
Copy link
Collaborator

@CodaFi CodaFi commented Mar 28, 2017

I don't have any qualms with not using GYB here - my thinking was that the file would be more resilient to future interface changes/tuple changes and we could update everything all at once. But that's a lot of work that can be done later. For now, LGTM.

@anayini
Copy link
Contributor Author

@anayini anayini commented Mar 28, 2017

There a bit of more discussion here: https://bugs.swift.org/browse/SR-4172, regarding the size of this code and whether the Comparable operators are needed. It also includes a helpful example for why the == operator could be useful to someone.

@CodaFi
Copy link
Collaborator

@CodaFi CodaFi commented Mar 29, 2017

@swift-ci please smoke test

@anayini
Copy link
Contributor Author

@anayini anayini commented Mar 29, 2017

Welp, I suck. Looks like a failure in IDE/complete_operators.swift, which I guess I didn't run locally. Will investigate at some point today

@moiseev
Copy link
Contributor

@moiseev moiseev commented Mar 29, 2017

@anayini that's why we have CI ;-) Few people have enough resources and patience to run all the tests locally.

@anayini
Copy link
Contributor Author

@anayini anayini commented Apr 24, 2017

Long time since last update, sorry about that. - @moiseev I'm actually still pretty confused about how the IDE code completion tests run using FileCheck. I've got something that I believe passes, but I'm definitely still confused about the semantics of the file.

I basically have a pseudo understanding of how it works, but would like a deeper understanding so that I can update my commit to make sense.

Arjun Nayini
@@ -40,7 +40,7 @@
// RUN: %target-swift-ide-test -code-completion -source-filename=%s -code-completion-token=INFIX_14 | %FileCheck %s -check-prefix=NO_OPERATORS
// RUN: %target-swift-ide-test -code-completion -source-filename=%s -code-completion-token=INFIX_15 | %FileCheck %s -check-prefix=NO_OPERATORS
// RUN: %target-swift-ide-test -code-completion -source-filename=%s -code-completion-token=INFIX_16 | %FileCheck %s -check-prefix=NO_OPERATORS
// RUN: %target-swift-ide-test -code-completion -source-filename=%s -code-completion-token=INFIX_17 | %FileCheck %s -check-prefix=NO_OPERATORS
// RUN: %target-swift-ide-test -code-completion -source-filename=%s -code-completion-token=INFIX_17 | %FileCheck %s -check-prefix=VOID_OPERATORS

This comment has been minimized.

@anayini

anayini Apr 24, 2017
Author Contributor

Should maybe rename VOID_OPERATORS to VOID_INFIX_OPERATORS?

func testInfix17(x: Void) {
x#^INFIX_17^#
}

// VOID_OPERATORS: Begin completions

This comment has been minimized.

@anayini

anayini Apr 24, 2017
Author Contributor

Generally want some feedback as to where the CHECK: lines need to go. Even before this PR seems like the NO_OPERATORS-NOT was in a strange place, but could be my misunderstanding.

This comment has been minimized.

@moiseev

moiseev Apr 25, 2017
Contributor

FileCheck is part of LLVM. See here for the documentation.

This comment has been minimized.

@moiseev

moiseev Apr 25, 2017
Contributor

Oh, I see how this is related to the change. () is same as Void, and therefore we get all the equality/comparison ops on the value of type Void. This might have other consequences...

This comment has been minimized.

@anayini

anayini Apr 25, 2017
Author Contributor

Yup. Took me a bit to figure out, but yeah SourceKit code completion was offering suggestions of all the equality/comparison ops that were added, but it wasn't expecting any suggestions so this test was failing.

This comment has been minimized.

@moiseev
Copy link
Contributor

@moiseev moiseev commented Apr 25, 2017

@swift-ci Please smoke test

@@ -244,6 +244,7 @@ func testInfix11() {
S2#^INFIX_11^#
}
// NO_OPERATORS-NOT: Decl[InfixOperatorFunction]

This comment has been minimized.

@moiseev

moiseev Apr 25, 2017
Contributor

Why the newline?

This comment has been minimized.

@anayini

anayini Apr 25, 2017
Author Contributor

Ah sorry - I can remove that.

@anayini
Copy link
Contributor Author

@anayini anayini commented May 1, 2017

bump @airspeedswift

I can fix the newline nits, but wanted to see if you had any other feedback if you have some time
:)

@anayini
Copy link
Contributor Author

@anayini anayini commented May 5, 2017

@moiseev - anyone else we can get feedback from :)

I'll push up the whitespace fix, but was going to wait for other feedback in case its needed.

@moiseev
Copy link
Contributor

@moiseev moiseev commented May 6, 2017

@anayini Sorry for the silence. Last I remember there were some discussions about it. We'll get back to you once things clear up. Thanks for your patience!

@anayini
Copy link
Contributor Author

@anayini anayini commented May 18, 2017

@moiseev - Just checking in again :)

@anayini
Copy link
Contributor Author

@anayini anayini commented Jun 3, 2017

@moiseev - Did ya'll come to a conclusion? Should I close for now?

@moiseev
Copy link
Contributor

@moiseev moiseev commented Jun 7, 2017

@swift-ci please smoke test

@moiseev
Copy link
Contributor

@moiseev moiseev commented Jun 7, 2017

@swift-ci Please Test Source Compatibility

@anayini
Copy link
Contributor Author

@anayini anayini commented Jun 13, 2017

@moiseev - Can dive into the Source Compatibility Suite error log - do these tests ever flake? If so, can you rerun?

@jrose-apple
Copy link
Contributor

@jrose-apple jrose-apple commented Jun 13, 2017

This one's a known issue that we just XFAILed. I'll kick off another run for you.

@swift-ci Please test source compatibility

@anayini
Copy link
Contributor Author

@anayini anayini commented Jun 14, 2017

Hm seems like this time its an IndexError: list index out of range from
xfail['branch'][swift_branch].split()[0]

@jrose-apple
Copy link
Contributor

@jrose-apple jrose-apple commented Jun 14, 2017

Finally back to normal!

@swift-ci Please test source compatibility

@anayini
Copy link
Contributor Author

@anayini anayini commented Jun 14, 2017

😄 . All green!

@anayini
Copy link
Contributor Author

@anayini anayini commented Jun 15, 2017

@moiseev - Are we good to merge?

@moiseev
Copy link
Contributor

@moiseev moiseev commented Jun 15, 2017

@anayini, do you mind sending an email to swift-evolution with a proposal to refine https://github.com/apple/swift-evolution/blob/master/proposals/0015-tuple-comparison-operators.md to cover the empty tuple case?

@anayini
Copy link
Contributor Author

@anayini anayini commented Jul 9, 2017

@moiseev - Sorry for the delay - was getting ready for a long OOTO and am now out of the country until the end of the month. Will be able to check in on this periodically. Sent the email.

@anayini
Copy link
Contributor Author

@anayini anayini commented Jul 12, 2017

@moiseev - Looks like it was accepted to be posted on swift-evolution. What are the next steps?

@moiseev moiseev merged commit ce53a65 into apple:master Jul 12, 2017
3 checks passed
3 checks passed
@swift-ci
Swift Source Compatibility Suite on macOS Platform Build finished.
Details
@swift-ci
Swift Test Linux Platform (smoke test)
Details
@swift-ci
Swift Test OS X Platform (smoke test)
Details
@moiseev
Copy link
Contributor

@moiseev moiseev commented Jul 12, 2017

@anayini Thanks for the contribution and your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants