Skip to content

Local immutable arrays#156

Closed
antalsz wants to merge 35 commits intoocaml-flambda:mainfrom
antalsz:local-immutable-arrays
Closed

Local immutable arrays#156
antalsz wants to merge 35 commits intoocaml-flambda:mainfrom
antalsz:local-immutable-arrays

Conversation

@antalsz
Copy link
Copy Markdown
Contributor

@antalsz antalsz commented Mar 24, 2023

Comprehensions are coming in a separate PR

@antalsz antalsz requested a review from goldfirere March 24, 2023 14:53
goldfirere
goldfirere reviewed Apr 25, 2023
View reviewed changes
@antalsz
Copy link
Copy Markdown
Contributor Author

antalsz commented Apr 27, 2023

This is all wrapped up except for rebasing as of 3be589b

@antalsz antalsz force-pushed the local-immutable-arrays branch 2 times, most recently from 5c807a5 to c3a9510 Compare April 27, 2023 21:56
@goldfirere
Copy link
Copy Markdown
Contributor

goldfirere commented Apr 28, 2023

As discussed just as I was leaving yesterday, while I would still like a quick comment from e.g. @riaqn, I think this is OK to merge.... except for the CI failures above.

@antalsz antalsz force-pushed the local-immutable-arrays branch 2 times, most recently from bee0f4f to c8a7a4a Compare May 1, 2023 20:06
antalsz added 20 commits May 1, 2023 16:06
@antalsz
Move caml_{i,}array_of_{i,}array primitives to array.c
2109585
@antalsz
Make immutable array externals local-aware
f5f4d28
@antalsz
Make immutable array literals support local contents
150a80f
@antalsz
Add %empty_iarray
7910bf0
@antalsz
Bootstrap
5283bd9
@antalsz
Implement most of the local iarray API
a0c31ef
@antalsz
Give Iarray.init a local callback
61b9ce0
@antalsz
Comments and small responses to Richard
8c78456
@antalsz
Revert "Add %empty_iarray"
23533e8 
This reverts commit 70f6c0d.
@antalsz
Replace references to empty_iarray with unsafe_of_array [||]
b08f59d
@antalsz
Text updates
ed6918a
@antalsz
Bootstrap
dd4cf4d
@antalsz
Comment clarification
992a2c4
@antalsz
Remove superfluous unsafe_sub_of_array_local
ae92f18
@antalsz
Add some missing iarray functions by avoiding let mutable
1d70f99
@antalsz
Drop some CRs
a0c0a05
@antalsz
Resynchronize iarray.mli and iarrayLabels.mli
dff79aa
@antalsz
Make iarray.ml and iarrayLabels.mli being copies more prominent
0161fff
@antalsz
Fix comment
3ca4d2b
@antalsz
[minor] Rename a local C variable
9108b47
@antalsz antalsz force-pushed the local-immutable-arrays branch 2 times, most recently from 07cd20d to 5e33bda Compare May 1, 2023 20:21
@antalsz antalsz force-pushed the local-immutable-arrays branch from 5e33bda to 46db5db Compare May 1, 2023 21:27
@antalsz antalsz force-pushed the local-immutable-arrays branch from 46db5db to ec81c8f Compare May 1, 2023 22:16
@antalsz antalsz force-pushed the local-immutable-arrays branch from ec81c8f to caff517 Compare May 1, 2023 22:23
@antalsz
Copy link
Copy Markdown
Contributor Author

antalsz commented May 2, 2023

This is ready for re-review now that I've made the array get/set primitives mode-aware; we should also get somebody who understands backend stuff (and maybe modes?) to review those parts of it given those changes

@mshinwell
Copy link
Copy Markdown
Contributor

mshinwell commented May 23, 2023

I'm reading the middle/backend parts, sorry for the delay

mshinwell
mshinwell requested changes May 25, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mshinwell mshinwell left a comment

Choose a reason for hiding this comment

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

I've read everything except stdlib/, testsuite/ and typing/ which I think someone else should look at.

@mshinwell
Copy link
Copy Markdown
Contributor

mshinwell commented May 25, 2023

Once comments have been addressed, please turn this into an flambda-backend PR: we should do the flambda2 implementation before this is merged.

@antalsz antalsz mentioned this pull request May 26, 2023
Merged
@antalsz
Copy link
Copy Markdown
Contributor Author

antalsz commented May 26, 2023

I'm going to leave this open for now but @mshinwell, I've responded to all your review (with one open question) and have moved this over to oxcaml/oxcaml#1420

mshinwell
mshinwell reviewed May 29, 2023
View reviewed changes
mshinwell
mshinwell requested changes May 29, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mshinwell mshinwell left a comment

Choose a reason for hiding this comment

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

Just a couple of minor things.

@antalsz
Copy link
Copy Markdown
Contributor Author

antalsz commented Jun 13, 2023

Closing in favor of oxcaml/oxcaml#1420

@antalsz antalsz closed this Jun 13, 2023
@goldfirere goldfirere mentioned this pull request Sep 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mshinwell mshinwell mshinwell requested changes

@goldfirere goldfirere goldfirere left review comments

@lpw25 lpw25 lpw25 left review comments

+1 more reviewer

@riaqn riaqn riaqn left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@antalsz @goldfirere @mshinwell @riaqn @lpw25