Implement tuple comparison operators #408

lilyball · 2015-12-10T22:43:59Z

This is an implementation of SE-0015 for tuples of arities 2–6.

lilyball · 2015-12-10T22:44:28Z

Proposal has not been accepted yet, so this should be marked as pending. I'm just submitting it now to make it available if anyone wants to look at it.

jckarter · 2015-12-10T22:53:59Z

Thanks @kballard ! Marked pending as requested.

gribozavr · 2015-12-10T23:10:03Z

test/1_stdlib/Tuple.swift

+// all the tuple types use the same basic implementation for the operators
+// so testing any arity tests them all
+
+func testEquality<A: Equatable, B: Equatable, C: Equatable>(


In the standard library code, we put spaces on both sides of the colon when it represents an 'is-a' relationship (in generic constraints, in inheritance and protocol implementation clauses etc.)

gribozavr · 2015-12-10T23:12:03Z

I would recommend that you do try calling every function at least once. There are a lot of components that can break in the compiler when using unusual types (e.g., name mangling, debug information, optimizer, IR generation, LLVM etc.)

gribozavr · 2015-12-10T23:13:01Z

stdlib/public/core/Tuple.swift.gyb

+%   for i in range(arity-1):
+  if lhs.${i} != rhs.${i} { return lhs.${i} ${op} rhs.${i} }
+%   end
+  return lhs.${arity-1} ${op}${opeq} rhs.${arity-1}


As a code size optimization, you could try defining one operators in terms of others.

I tried defining != in terms of = and it actually increased code size. I didn't do any further investigation.

That's unfortunate. Thanks for checking!

lilyball · 2015-12-10T23:31:22Z

I'll update it based on feedback later today (calling each function once, adding spaces).

lilyball · 2015-12-10T23:39:23Z

Incidentally, I ran build-script -R -i -T and got the following failure:

******************** TEST 'Swift :: IDE/complete_expr_tuple.swift' FAILED ********************
Script:
--
…
--
Exit Code: 1

Command Output (stderr):
--
found code completion token TUPLE_NO_DOT_1 at offset 1425
/Users/kevin/Dev/Swift/Apple/swift/test/IDE/complete_expr_tuple.swift:30:20: error: expected string not found in input
// TUPLE_NO_DOT_1: Begin completions, 2 items
                   ^
<stdin>:1:1: note: scanning from here
found code completion token TUPLE_NO_DOT_1 at offset 1425
^
<stdin>:2:1: note: possible intended match here
Begin completions, 8 items
^

It appears to be testing code completion for tuple types. After checking the code completion suggestions in the swift REPL, I'm assuming that it's erroring because completion now suggests the 6 comparison operators in addition to the .0 and .1 fields.

Should I just update this to expect 8 items instead of 2, with relevant patterns? Or is the test explicitly trying to check tuples that have no other members, which would require altering the tuple to include a non-Equatable type (and changing all the patterns)?

gribozavr · 2015-12-11T00:24:59Z

@kballard I think updating the test with new patterns is the right thing. Operator + tuple combination is a rare one that I don't think we have covered in our code completion tests.

lilyball · 2015-12-11T03:56:29Z

Updated PR for feedback. Still need to update the code completion tests though.

lilyball · 2015-12-12T03:19:29Z

Updated code completion tests. I do have a question about it though which I will put as a file comment.

lilyball · 2015-12-12T03:23:41Z

test/IDE/complete_expr_tuple.swift

+// TUPLE_NO_DOT_1-NEXT: Decl[InfixOperatorFunction]/OtherModule[Swift]: >= {#(Int, Double)#}[#Bool#]{{; name=.+$}}
+// TUPLE_NO_DOT_1-NEXT: Decl[InfixOperatorFunction]/OtherModule[Swift]: < {#(Int, Double)#}[#Bool#]{{; name=.+$}}
+// TUPLE_NO_DOT_1-NEXT: Decl[InfixOperatorFunction]/OtherModule[Swift]: != {#(Int, Double)#}[#Bool#]{{; name=.+$}}
+// TUPLE_NO_DOT_1-NEXT: Decl[InfixOperatorFunction]/OtherModule[Swift]: > {#(Int, Double)#}[#Bool#]{{; name=.+$}}


How are code completions sorted? I tried looking through the code but it's not immediately obvious. I did find code that appears to sort them based on some definition of a name (which I'd assume is like "==" but I don't actually know), but the output here doesn't appear to be sorted in any way I can figure out. If it was sorted by name I'd expect to see the ordering be !=, <, <=, ==, >, >=, and it's not. Heck, != should show up before .0 then (and in fact when tab-completing in the integrated REPL it does sort it like that). So the ordering here must be based on something else, and I'm worried that this "something else" might not be stable.

I'm also not sure if we really want to match the full Decl[...]/OtherModule[..] prefix, but I'm not as concerned about that.

Ideally I'd actually switch to using -DAG patterns to make it unordered, but that loses the property of ensuring there's no lines before the End completions that don't match one of the expected completions. I don't know how important that is.

The completions are ordered in name lookup order (i.e., unpredictable).

You should switch to using -DAG. The extra completions issue is handled by checking the number of completions in the Begin completions line.

Ok. Should I switch them all to -DAG, under the assumption that they may plausibly show up before the tuple fields? The existing tests were already relying on the order of the tuple fields, and I don't know if that's something they need to preserve (but preserving that requires assuming they show up first).

Yes, the order does not matter for compiler correctness.

lilyball · 2015-12-12T05:59:07Z

Updated again to use -DAG patterns. I used -DAG for the .0 and .1 patterns too, under the assumption that operators could plausibly be emitted before fields.

RoyalIcing · 2015-12-12T08:19:43Z

What if there was a new syntax added to make checking each member of type quicker to write? That would be useful for structs, enums with associated values, and tuples? Either a new syntax for repeating an operator on multiple members, or auto completion, or both?

lilyball · 2015-12-12T08:37:36Z

Sounds like you're proposing an extremely specific ad-hoc macro. And I don't know how you can automate this in the general case anyway, because I don't think it's true that arbitrary operations that you want to implement on tuples would all have the form of if lhs.0 OP1 rhs.0 { return lhs.0 OP2 rhs.0 }; if lhs.1 OP rhs.1 { return lhs.1 OP2 rhs.1 }; .... Heck, just look at the == and != implementations to see other forms.

In any case, if you want to think about adding a macro system to Swift, feel free to discuss it on swift-evolution. This PR doesn't really seem like the appropriate place for that, since a macro system is something that would take a while to develop and implement and there's no point in blocking this PR on it.

gribozavr · 2015-12-24T01:19:19Z

Could you add doc comments to the new APIs?

lilyball · 2015-12-24T01:21:45Z

@gribozavr AFAIK the only existing implementation of < that has a doc comment is the one on _Strideable. That said, it doesn't hurt to add comments here, so I'll go ahead and do that.

gribozavr · 2015-12-24T01:27:18Z

test/1_stdlib/Tuple.swift.gyb

+    case .GreaterThan: return .LessThan
+    case .UnorderedWith: return .UnorderedWith
+    }
+  }


I think this function is not used.

You're right. I created it because I was using existing floating-point tests as inspiration, but didn't end up using it because there was no reason to do so.

gribozavr · 2015-12-24T01:29:59Z

Yes, it is worth explaining the semantics of these operations.

gribozavr · 2015-12-24T01:32:00Z

The patch LGTM otherwise.

lilyball · 2015-12-24T01:33:10Z

I'm tempted to just document the < operator, because that's the only one in Comparable that has documentation, and because I'm probably going to say something like "A lexicographic order over tuples of Comparable elements".

gribozavr · 2015-12-24T01:34:15Z

I would defer reviewing the documentation to @dabrahams.

lilyball · 2015-12-24T01:46:48Z

Rebased, squshed tests into initial commit, and updated based on feedback.

dabrahams · 2015-12-24T01:57:36Z

Kevin, this is great, but I’d like to see a little more in the doc comments. In particular, explain (in English) when two tuples are considered equal. Also, please do the same for the ordering comparisons (or provide examples) so that users who don’t know what lexicographical ordering is don’t have to look up and understand the definition just to get an idea of what this means.

Thanks!

lilyball · 2015-12-24T02:02:39Z

Ok, I can do that. I was using the existing documentation of Comparable as an example, where it links to the wikipedia page for "strict total order". I also skipped == because I figured it was a reasonably obvious definition, but I can document it too.

Do you want me to add a doc string to every single operator, or is it sufficient to have just the docstring on < and ==? My concern with adding it for every single operator is it would basically be a repetition of the same docstring over and over, which I'm assuming is why Comparable only bothers to document < (that and the definitions of the other operators can all be derived from the definitions of < and ==).

dabrahams · 2015-12-24T02:04:10Z

Heh, the sins of the past (ours) don’t justify those of the future ;-)

I’m afraid they should all be documented. Fortunately you have gyb to cut the repetition in source.

lilyball · 2015-12-24T02:04:56Z

Fair enough.

Implement == and != for tuples up to arity 6 where each component type is Equatable. Implement <, <=, >, and >= for tuples up to arity 6 where each component type is Comparable.

lilyball · 2015-12-24T02:25:52Z

@dabrahams PR updated with docstrings for every operator.

dabrahams · 2015-12-24T02:28:23Z

Nice, thanks!

Implement tuple comparison operators

RoyalIcing · 2015-12-24T03:51:54Z

Nice one!

tests: fall back to fork() if posix_spawnp() is not available

[pull] swiftwasm from master

Add flag to xcodebuild to disable manifest sandbox

lilyball mentioned this pull request Dec 10, 2015

Proposal for tuple comparison operators swiftlang/swift-evolution#45

Merged

jckarter added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Dec 10, 2015

gribozavr reviewed Dec 10, 2015
View reviewed changes

lilyball force-pushed the tuple-comparison-ops branch from 41bd040 to 5e62134 Compare December 11, 2015 03:56

lilyball reviewed Dec 12, 2015
View reviewed changes

gribozavr added swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process and removed swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review labels Dec 24, 2015

gribozavr reviewed Dec 24, 2015
View reviewed changes

lilyball force-pushed the tuple-comparison-ops branch from 55d4094 to 20e214d Compare December 24, 2015 01:46

[Stdlib] Implement comparison operators for tuples

b61c7a5

Implement == and != for tuples up to arity 6 where each component type is Equatable. Implement <, <=, >, and >= for tuples up to arity 6 where each component type is Comparable.

lilyball force-pushed the tuple-comparison-ops branch from 20e214d to b61c7a5 Compare December 24, 2015 02:25

dabrahams pushed a commit that referenced this pull request Dec 24, 2015

Merge pull request #408 from kballard/tuple-comparison-ops

3576996

Implement tuple comparison operators

dabrahams merged commit 3576996 into swiftlang:master Dec 24, 2015

lilyball deleted the tuple-comparison-ops branch December 24, 2015 02:29

slavapestov pushed a commit to slavapestov/swift that referenced this pull request Nov 27, 2018

Merge pull request swiftlang#408 from adierking/fork

6a5c6d8

tests: fall back to fork() if posix_spawnp() is not available

kateinoigakukun pushed a commit to kateinoigakukun/swift that referenced this pull request Mar 20, 2020

Merge pull request swiftlang#408 from swiftwasm/master

4d4361c

[pull] swiftwasm from master

freak4pc pushed a commit to freak4pc/swift that referenced this pull request Sep 28, 2022

Merge pull request swiftlang#408 from apple/add-xcodebuild-flag

704fd5f

Add flag to xcodebuild to disable manifest sandbox

Implement tuple comparison operators #408

Implement tuple comparison operators #408

Conversation

lilyball commented Dec 10, 2015

lilyball commented Dec 10, 2015

jckarter commented Dec 10, 2015

Choose a reason for hiding this comment

gribozavr commented Dec 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilyball commented Dec 10, 2015

lilyball commented Dec 10, 2015

gribozavr commented Dec 11, 2015

lilyball commented Dec 11, 2015

lilyball commented Dec 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilyball commented Dec 12, 2015

RoyalIcing commented Dec 12, 2015

lilyball commented Dec 12, 2015

gribozavr commented Dec 24, 2015

lilyball commented Dec 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribozavr commented Dec 24, 2015

gribozavr commented Dec 24, 2015

lilyball commented Dec 24, 2015

gribozavr commented Dec 24, 2015

lilyball commented Dec 24, 2015

dabrahams commented Dec 24, 2015

lilyball commented Dec 24, 2015

dabrahams commented Dec 24, 2015

lilyball commented Dec 24, 2015

lilyball commented Dec 24, 2015

dabrahams commented Dec 24, 2015

RoyalIcing commented Dec 24, 2015