[copy-operator] Add support for the copy operator in preparation for making consuming and borrowing no implicit copy. #65935
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gottesmm
requested review from
hborla,
slavapestov,
xedin,
ahoppen,
bnbarham,
CodaFi,
DougGregor,
rintaro and
tshortli
as code owners
kavon
reviewed
May 16, 2023
jckarter
approved these changes
May 16, 2023
ahoppen
reviewed
May 16, 2023
gottesmm
force-pushed
the
rdar101862423
branch
2 times, most recently
from
d14e9eb to
f503da1
Compare
|
@swift-ci smoke test |
|
@swift-ci smoke test |
…making consuming and borrowing no implicit copy. Some notes: 1. I implemented this as a contextual keyword that can only apply directly to lvalues. This ensures that we can still call functions called copy, define variables named copy, etc. I added tests for both the c++ and swift-syntax based parsers to validate this. So there shouldn't be any source breaks. 2. I did a little bit of type checker work to ensure that we do not treat copy_expr's result as an lvalue. Otherwise, one could call mutating functions on it or assign to it, which we do not want since the result of copy_value is 3. As expected, by creating a specific expr, I was able to have much greater control of the SILGen codegen and thus eliminate extraneous copies and other weirdness than if we used a function and had to go through SILGenApply. rdar://101862423
…identifiers correctly. This resulted from explorations around implementing the copy expr. rdar://109479131
…d an lvalue. Before this change it was possible to: 1. Call mutating methods on a consume result. 2. assign into a consume (e.x.: var x = ...; (consume x) = value. From an implementation perspective, this involved just taking the logic I already used for the CopyExpr and reusing it for ConsumeExpr with some small tweaks. rdar://109479440
…on var without errors to work. I found that in the given case, an extra allocation was being emitted causing us to in the success case to have an extra copy. In the previous commit, as part of updating the codegen for consume, I also ensured that we wouldn't have that extra allocation any more by handling the lvalue directly. rdar://109222496
|
@jckarter you probably want to take a quick additional look at this since I decided while I was here to also fix consume so it has the same behavior and also in the process fix another issue where consume was emitting proper errors for move only types, but due to an extra allocation that we introduced in the var case (due to us using rvalue logic instead of lvalue logic that the type checker work here allows us to), we would hit a copy of noncopyable type error. Now that we use the lvalue logic directly for consume like I do for copy, that is elided. |
ahoppen
reviewed
May 18, 2023
test/IDE/copy_expr.swift
Outdated
There was a problem hiding this comment.
Just in case you’re doing another revision of this PR: The general naming pattern of these files would be complete_copy_expr.swift
|
I updated the tests in lldb so that they work with these changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
rdar://101862423