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: add Bitvec reverse definition, getLsbD_reverse, getMsbD_reverse, reverse_append, reverse_replicate and Nat.mod_sub_eq_sub_mod #6476

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

luisacicolini
Copy link
Contributor

@luisacicolini luisacicolini commented Dec 30, 2024

This PR defines reverse for bitvectors and implements a first subset of theorems (getLsbD_reverse, getMsbD_reverse, reverse_append, reverse_replicate) and an additional theorem necessary for one of the proofs (Nat.mod_sub_eq_sub_mod).

The main objective is to simplify the proofs in #6326. I sadly could not find a way to avoid adding Nat.mod_sub_eq_sub_mod, any suggestion in this direction would be greatly helpful.

@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 Dec 31, 2024
@leanprover-community-bot
Copy link
Collaborator

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 9b28c5879a77f9f0212a7f39ea83f56c71abda42 --onto 2c87905d77b707c3283c1de759fd63269ac386a1. (2024-12-31 15:59:57)

@@ -669,4 +669,11 @@ def ofBoolListLE : (bs : List Bool) → BitVec bs.length
| [] => 0#0
| b :: bs => concat (ofBoolListLE bs) b

/- ### reverse -/

/-- Reverse of a Bitvec. We treat BitVec as lists of bools. -/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/-- Reverse of a Bitvec. We treat BitVec as lists of bools. -/
/-- Reverse the bits in a bitvector. -/

We generally phrase a docstring for a function as an action/verb rather than just the result/noun.

I also wouldn't mention that we treat a bitvector as a list of bools, that's the "expected" interpretation of a bitvector (list of bools is more or less the same as a "vector of bits").

I guess you're referring here to the fact that the implementation is more in the style of a List.reverse versus other definitions that rely on the Nat-based internal encoding. That kind of comment feels more appropriate as a local comment rather than a docstring, but even then, I'm not sure it needs mentioning; it's clear from the code already.

/- ### reverse -/

/-- Reverse of a Bitvec. We treat BitVec as lists of bools. -/
def reverse : (w : Nat) → BitVec w → BitVec w
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def reverse : (w : Nat) → BitVec w → BitVec w
def reverse : {w : Nat} → BitVec w → BitVec w

w can be inferred from the bitvector arg, and can thus be implicit

· simp only [hi, decide_true, show w - 1 - i < w by omega, Bool.true_and]
congr; omega
· simp [hi, show i ≥ w by omega]

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm missing a msb_reverse specialization

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.

3 participants