Fix toArray() for standard collections of non-default initializable records (lists, sets, …) #28259
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
…types --- Signed-off-by: Brad Chamberlain <[email protected]>
Member
Author
|
@jabraham17 : Would you be up for giving this a review? |
--- Signed-off-by: Brad Chamberlain <[email protected]>
…ap use While testing my new test, I found that it was getting weird behavior when running on multiple locales, due to having the map live on one locale and the task running the iterator on another. I'm not sure what's causing this, so added a future capturing the behavior and avoided the remote iteration in my new test for now. --- Signed-off-by: Brad Chamberlain <[email protected]>
--- Signed-off-by: Brad Chamberlain <[email protected]>
bradcray
changed the title
bradcray
changed the title
In re-reviewing my PR this morning, I realized that I'd mis-named the .bad file. Associating it with the future test (-loc.chpl) causes failures due to differing behaviors on different runs, so I'm just removing the .bad file instead. Meanwhile, I tightened up the SKIPIF to only apply to the future since the other case works with or without comm=none --- Signed-off-by: Brad Chamberlain <[email protected]>
Member
Author
|
@jabraham17 : As discussed on slack, I've expanded the original PR you reviewed to cover Map, Set, and Heap as well, and to add a future for a bug in Map that I ran across along the way. If you could re-review (or just review the diff between your last review and now), I'd appreciate it. |
jabraham17
approved these changes
Jan 7, 2026
--- Signed-off-by: Brad Chamberlain <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR rewrites
list.toArray()to avoid the use of an on-clause, and therefore the delayed initialization of the temporary array being returned, in order to resolve #28201 and #28202. It also adds a test based on the code in #28201, though extends it to do cross-locale calls to ensure that everything works in single- and multi-locale settings (acrosscomm=nonevs.!=nonetesting configurations).As noted in #28201 (comment), this rewrite could have an adverse impact on performance if the list is on a distinct locale from the task calling
.toArray(), but I think this is the right change all the same since (a) correctness and generality are more important than performance and (b) the case where the list and calling task are on distinct locales seems unlikely to be a frequent occurrence in practice. Moreover, for the case where the two are co-located, this should accelerate things by avoiding the creation/copy of a second array (though perhaps we would have successfully stolen it anyway?).Ultimately, if/when we care about optimizing performance, we should probably use bulk copies from the list's sub-arrays to the result array anyway, which would be faster for both the co-located and distributed cases.
That original fix was then propagated to the other standard collection types (sets, maps, heaps) and tests were added for them as well. The one case that's not as strong as the others is the map version, which results in undefined behavior when run on multiple locales due to #28266 and this PR includes a future in support of that (new) issue.
Resolves #28201
Resolves #28202