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: proposed change to BitVec API #5200

Merged
merged 11 commits into from
Aug 30, 2024
Merged

feat: proposed change to BitVec API #5200

merged 11 commits into from
Aug 30, 2024

Conversation

kim-em
Copy link
Collaborator

@kim-em kim-em commented Aug 29, 2024

This renames BitVec.getLsb to getLsbD (D for "default" value, i.e. false), and introduces getLsb? and getLsb' (which we can rename to getLsb after a deprecation cycle).

(Similarly for getMsb.)

Also adds a GetElem class so we can use x[i] and x[i]? notation.

Later, we will turn

theorem getLsbD_eq_getElem?_getD (x : BitVec w) (i : Nat) (h : i < w) :
    x.getLsbD i = x[i]?.getD false

on as a @[simp] lemma.

This PR doesn't attempt to demonstrate the benefits, but I think both arguments are going to get easier, and this will bring the BitVec API closer in line to List/Array, etc.

@kim-em kim-em requested a review from TwoFX August 29, 2024 06:04
@kim-em kim-em requested a review from hargoniX as a code owner August 29, 2024 06:04
@kim-em kim-em requested a review from leodemoura as a code owner August 29, 2024 06:20
@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 Aug 29, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Aug 29, 2024

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 44985dc9a64bb6f4f898abb7be24e7be47f42bb7 --onto 9ce15fb0c61e3a1bee17fd81647ed4d02b4f6c6d. (2024-08-29 06:37:58)
  • ❗ Mathlib CI can not be attempted yet, as the nightly-testing-2024-08-29 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-08-30 01:58:41)

@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 29, 2024 07:12 Inactive
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 29, 2024 07:45 Inactive
Copy link
Contributor

@alexkeizer alexkeizer left a comment

Choose a reason for hiding this comment

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

I like the getLsb -> getLsbD rename (in fact, I've proposed this exact change before, back when bitvectors were still in now-batteries/then-std4)

Comment on lines 119 to 124
/--
Return the `i`-th least significant bit.

This will be renamed `getLsb` after the existing deprecated alias is removed.
-/
@[inline] def getLsb' (x : BitVec w) (i : Nat) (_ : i < w) : Bool := x.toNat.testBit i
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might make sense to bundle the proof, and have this take an i : Fin w, akin to how List.get does it.
IIRC, there are some places we currently have getLsb i with i : Fin w (relying on the Fin to Nat coercion). If we bundle the proof in a single Fin argument, these call sites will just magically keep working

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed.

@kim-em kim-em requested a review from digama0 as a code owner August 30, 2024 01:41
@kim-em kim-em enabled auto-merge August 30, 2024 01:42
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 30, 2024 02:00 Inactive
@kim-em kim-em added this pull request to the merge queue Aug 30, 2024
Merged via the queue into master with commit 6b62fed Aug 30, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants