Skip to content
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

feat: usize for array types #4801

Merged
merged 2 commits into from
Jul 21, 2024
Merged

feat: usize for array types #4801

merged 2 commits into from
Jul 21, 2024

Conversation

fgdorais
Copy link
Contributor

@fgdorais fgdorais commented Jul 21, 2024

Add efficient usize functions for Array, ByteArray, FloatArray.

This is part 1 of 2 since there is a need to update stage0 between the two parts. (See discussion below.)

Closes #4654

@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Jul 21, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

Mathlib CI status (docs):

  • ❗ Mathlib CI can not be attempted yet, as the nightly-testing-2024-07-20 tag does not exist there yet. We will retry when you push more commits. If you rebase your branch onto nightly-with-mathlib, Mathlib CI should run now. (2024-07-21 04:52:18)

@fgdorais fgdorais changed the title feat: usize for arrays feat: usize for array types Jul 21, 2024
Add efficient usize functions for Array, ByteArray, FloatArray.

Closes leanprover#4654
@fgdorais fgdorais force-pushed the array-usize branch 3 times, most recently from f42a424 to 75c853e Compare July 21, 2024 06:44
@fgdorais
Copy link
Contributor Author

It was necessary to update stage0 in order to use the usize functions.

@fgdorais fgdorais marked this pull request as ready for review July 21, 2024 07:10
@fgdorais fgdorais requested a review from kim-em as a code owner July 21, 2024 07:10
@fgdorais
Copy link
Contributor Author

awaiting-review

@github-actions github-actions bot added the awaiting-review Waiting for someone to review the PR label Jul 21, 2024
@nomeata
Copy link
Collaborator

nomeata commented Jul 21, 2024

I didn't expect this to need a stage0 update. Why is that so?

@fgdorais
Copy link
Contributor Author

I didn't expect that either. It's because these are extern definitions so there is no code for them until stage0 is updated.

@nomeata
Copy link
Collaborator

nomeata commented Jul 21, 2024

Hmm, still somewhat strange, the the C symbol should already be there, shouldn’t it? Did you try setting prefer_native to true?

But I’m no expert here. I’m just bringing this up because PRs with stage0 updates need to be manually merged by (I think) Sebastian or Leo. If you’d split it in two PRs then more people can merge and it can go through the normal merge queue.

@fgdorais
Copy link
Contributor Author

I'm not an expert there either. If I don't update stage0 then calls to usize look for the reference implementation instead of the extern symbol, but the reference implementation is never compiled.

Yes, I read the manual. I also made sure the commits were clean. I was thinking about splitting in two but I was hoping Sebastian would say something before I did that. Either way, there needs to be a stage0 update between the two commits.

@nomeata
Copy link
Collaborator

nomeata commented Jul 21, 2024

Right, this works of course! It’s just that with two PRs, the stage0 update can happen via the github bot. But not problem for me letting Sebastian merge this PR :-)

@fgdorais
Copy link
Contributor Author

There's a bot for that! I thought Sebastian would have to do it either way...

I will split right away then!

@nomeata
Copy link
Collaborator

nomeata commented Jul 21, 2024

There is a bot these days. It runs automatically after a PR merge whenever it touches stage0/src/stdlib_flags.h, or anyone with commit rights can trigger it.

So you could even leave a comment in stage0/src/stdlib_flags.h of the form

// please update stage0

and it’ll happen automatically after the PR enters master (and it will nicely override this comment).

@nomeata nomeata enabled auto-merge July 21, 2024 10:12
@nomeata nomeata added this pull request to the merge queue Jul 21, 2024
@fgdorais
Copy link
Contributor Author

Thank you so much for your help Joachim! This PR would have stayed in limbo for a long time without your enlightening comments!

Merged via the queue into leanprover:master with commit 8f0631a Jul 21, 2024
12 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Jul 21, 2024
This is part 2 of 2 of #4801 (which closes #4654). That PR was split in
two to allow a stage0 update between declaring the `usize` functions and
using them where they are needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review Waiting for someone to review the PR toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Array.usize
3 participants