-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
[CSApply] Restructure the implicit AST when applying @dynamicCallable #23845
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. In the future, instead of splitting the code change and test change into separate commits, I would instead suggest one commit per bug fix. This way it is clear which code change corresponds to which bug fix.
@swift-ci Please smoke test |
@swift-ci Please test source compatibility |
Oops, I just found SR-9239 and completely missed SR-10313 - my bad. @pschuh made similar So there may be some duplicate code and code divergence across branches. Not a big deal, except that it'd be nice (for the Swift for TensorFlow team) if someone could unify the code across the branches to facilitate downstreaming changes from master to |
@dan-zheng Oh I hadn't noticed this was fixed on tensorflow. How would one go about unifying when this is ready? |
It should just be a matter of copy-pasting the new code from one branch to the other. The new code on Would you (please) like to help us unify code? cc @pschuh |
@slavapestov @dan-zheng alright this should be ready to go now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
@swift-ci Please smoke test |
add tests dont assume dict type dont assume member ref ast formatting more formatting Update getTypeWitnessByName
@jrose-apple I've fixed the compile error and added a test case for SR-10753 |
@swift-ci Please test |
@swift-ci Please test source compatibility |
Build failed |
Build failed |
[CSApply] Restructure the implicit AST when applying @dynamicCallable
Solves: SR-9239 and SR-10313
cc: @slavapestov @xedin