Prevent modification of const ref varargs, too #25902
Merged
Conversation
This file contains 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
Modifies the same place as the PRs for const and const in, but additionally needed a fix for `moveSetConstFlagsAndCheck` because the local tuple variable was using a `PRIM_GET_MEMBER_VALUE` instead of a `PRIM_GET_MEMBER`. We'll see if that has broader consequences, though. ---- Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
Covers both queried and unqueried numbers of arguments ---- Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
…works This test failed due to us now (accurately) detecting that `chpl__buildAssociativeArrayExpr` violates the const-ness of the varargs for owned arguments. In talking with Michael, we decided it was okay for this to get futurized as a result, and added a version of the test that does not fail, showing that you can still create associative arrays that store owned classes (just not using associative array literals) ---- Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
…e merged This is basically the same thing as removing the deprecated behavior (for the purpose of this particular test, we need to perform the behavior update more generally still), so I don't think the change is concerning ---- Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
lydia-duncan
added
stat: Needs Code Review
and removed
stat: Waiting on Contributor
labels
mppf
reviewed
Oct 9, 2024
---- Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
…LUE` Michael pointed out that `PRIM_GET_MEMBER_VALUE` can situationally return a ref or a value and we don't want to apply the const-ness in the value case (as it will be a copy and thus independent of the behavior of the `const` source). So ensure that the result of the call will be a ref before propagating the `const`. This passed the normally impacted tests locally, will run a full paratest as well ---- Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
mppf
approved these changes
Oct 9, 2024
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.
Resolves #25901
Modifies the same place as the PRs for const and const in, but additionally needed a fix for
moveSetConstFlagsAndCheckbecause the local tuple variable was using aPRIM_GET_MEMBER_VALUEinstead of aPRIM_GET_MEMBER(though only in the case where it returns a ref).Also updates a couple of tests that were impacted by the change:
chpl__buildAssociativeArrayExprshould still be generally usingconstfor its varargs, and copying an owned class does modify the original) and was not caught before this change. Add test locking in that there's an alternate way to create an associative array of owned classes that doesn't use the associative array literal syntax. I also opened [Bug]: support for associative array literals with owned classes depended on violating const checks for varargs #26035 to track the futureAdd tests locking in the fix for const ref. Covers both queried and unqueried numbers of arguments.