Skip to content

Conversation

@dan-zheng
Copy link
Contributor

Remove call declarations.
Treat methods named func call as call-syntax delegate methods.

@dan-zheng dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Apr 18, 2019
@dan-zheng
Copy link
Contributor Author

The majority of the diff comes from reverting #24086.
The actual reimplementation is <100 LOC.

@dan-zheng dan-zheng force-pushed the tf-callable-revamp branch 2 times, most recently from 41eec3d to 1990111 Compare April 18, 2019 16:00
dan-zheng added a commit to tensorflow/swift-apis that referenced this pull request Apr 18, 2019
@dan-zheng dan-zheng force-pushed the tf-callable-revamp branch from 1990111 to 463c4dc Compare April 18, 2019 16:34
@dan-zheng dan-zheng requested review from lattner, rxwei and saeta April 18, 2019 16:36
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider adding a IsCallable bit to FuncDecl, instead of checking "callable-ness" on the fly.
That requires further decl/serialization changes - we can make that change after committing our desired func call API.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend not doing that, unless this is a particularly hot predicate. Cached bits like this have to be kept in sync with the ast. Also, instead of serializing it, if the bit needs to be stored, it can always be computed in the module reader.

@dan-zheng
Copy link
Contributor Author

@swift-ci Please test tensorflow

@dan-zheng dan-zheng force-pushed the tf-callable-revamp branch from 463c4dc to ca823b8 Compare April 18, 2019 16:58
@dan-zheng
Copy link
Contributor Author

@swift-ci Please test tensorflow

Copy link
Contributor

@rxwei rxwei left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

@dan-zheng dan-zheng force-pushed the tf-callable-revamp branch 2 times, most recently from aa1a0c0 to 4734e13 Compare April 18, 2019 17:24
dan-zheng added a commit to tensorflow/swift-apis that referenced this pull request Apr 18, 2019
dan-zheng added a commit to tensorflow/swift-apis that referenced this pull request Apr 18, 2019
Remove `call` declarations.
Treat methods named `func call` as call-syntax delegate methods.
@dan-zheng dan-zheng force-pushed the tf-callable-revamp branch from 4734e13 to 4382b73 Compare April 18, 2019 17:38
@dan-zheng
Copy link
Contributor Author

@swift-ci Please clean test tensorflow

@dan-zheng
Copy link
Contributor Author

@swift-ci Please clean test tensorflow macOS

@rxwei
Copy link
Contributor

rxwei commented Apr 18, 2019

We should add tests in combination with other language features such as IUOs and try.

struct CallableStuff {
  func call() {}
  func call(_ f: () throws -> ()) rethrows {}
}

var x: CallableStuff! = CallableStuff()
x() // should work
x!() // should work
x {} // should work
try x { throw ... } // should work

@rxwei rxwei merged commit ec27d9b into swiftlang:tensorflow Apr 18, 2019
@dan-zheng
Copy link
Contributor Author

We should add tests in combination with other language features such as IUOs and try.

Added more tests along those lines in #24157.

@lattner
Copy link
Contributor

lattner commented Apr 20, 2019

Much simpler Dan, great work!

rxwei pushed a commit to rxwei/swift that referenced this pull request May 11, 2019
* Revert "[TF] Introduce callables. (swiftlang#24086)"

This reverts commit f7c7edf.

* Revert "Fix `call` declaration serialization. (swiftlang#24121)"

This reverts commit 6466a84.

* Reimplement callables using `func call`.

Remove `call` declarations.
Treat methods named `func call` as call-syntax delegate methods.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tensorflow This is for "tensorflow" branch PRs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants