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

Coq proof lib #386

Closed
wants to merge 12 commits into from
Closed

Coq proof lib #386

wants to merge 12 commits into from

Conversation

cmester0
Copy link
Contributor

@cmester0 cmester0 commented Dec 7, 2023

Add coq library files.

@cmester0 cmester0 marked this pull request as draft December 7, 2023 13:15
@cmester0 cmester0 marked this pull request as ready for review December 7, 2023 13:46
@spitters
Copy link
Contributor

spitters commented Dec 7, 2023

LGTM. Do we need MachineIntegers.v? The readme says to install it via opam.

@cmester0
Copy link
Contributor Author

cmester0 commented Jan 6, 2024

From what I can see MachineIntergers.v file is not directly part of CompCert library, so we do need it here.

From Hacspec Require Export MachineIntegers.
From Hacspec Require Export Hacspec_Lib.

(** Should be moved to Hacspec_Lib.v **)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this comment?

Notation get_exact_chunk := seq_get_exact_chunk.
Definition set_chunk {a: Type} `{Default a} {len} (s: seq a) {WS} (chunk_len: @int WS) (chunk_num: @int WS) (chunk: array_or_seq a len) : seq a := seq_set_chunk s (unsigned chunk_len) (unsigned chunk_num) (as_seq chunk).
Definition set_exact_chunk {a} `{H : Default a} {len} s {WS} := @set_chunk a H len s WS.
Notation get_remainder_chunk := seq_get_remainder_chunk.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: remove spaces

end).
Definition g_public_byte_seq : G (public_byte_seq) :=
listOf arbitrary.
(* @genList int8 gen_int8. *)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these comments?

@spitters
Copy link
Contributor

spitters commented Jan 7, 2024

From what I can see MachineIntergers.v file is not directly part of CompCert library, so we do need it here.

Interesting. So, where does it come from then? If possible, we should try to avoid code duplication.

@cmester0
Copy link
Contributor Author

From what I can see MachineIntergers.v file is not directly part of CompCert library, so we do need it here.

Interesting. So, where does it come from then? If possible, we should try to avoid code duplication.

Hmm it might be here https://github.com/AbsInt/CompCert/blob/master/lib/Integers.v , however it seems modifications been made to that file, as modules are used, while we just need a typeclass.

@spitters
Copy link
Contributor

Yes, that seems like a likely source.
It's possible that we are just using an old version.
For the last yard, we've switched to the math-comp integers. It would be good to switch to the new stdlib one that's in preparation when it becomes available.

@W95Psp
Copy link
Collaborator

W95Psp commented Mar 7, 2024

Hi @cmester0, what's the status of this PR? seems to be mergeable, no?

@cmester0
Copy link
Contributor Author

cmester0 commented Mar 7, 2024

Yes, but I would like to add some testing, and structure the library a bit, but I guess those can be follow up PRs.

@spitters
Copy link
Contributor

Why are you going back to the compcert integers? The jasmin/math-comp ones are needed for the jasmin connection.

Getting this merged would help us make a choice. Moreover, that library comes from fiat-crypto, which we also connect to with hacspec.
coq/coq#17043

@cmester0
Copy link
Contributor Author

We have been using CompCert integers for the Coq backend, I was just updating them, to remove the weird extra file (MachineIntegers.v). If there is a better or more standard implementation, I can change the implementation to use that.

@spitters
Copy link
Contributor

spitters commented Mar 25, 2024 via email

@cmester0 cmester0 force-pushed the coq_lib branch 2 times, most recently from 7fde53b to d2fbf65 Compare April 9, 2024 16:48
@W95Psp W95Psp marked this pull request as draft April 11, 2024 11:35
auto-merge was automatically disabled April 11, 2024 11:35

Pull request was converted to draft

@W95Psp
Copy link
Collaborator

W95Psp commented Apr 11, 2024

Converting this PR to a draft for now, @cmester0 can you undraft it whenever you want to merge it?

Copy link

github-actions bot commented Oct 3, 2024

This PR has been marked as stale due to a lack of activity for 60 days. If you believe this pull request is still relevant, please provide an update or comment to keep it open. Otherwise, it will be closed in 7 days.

@github-actions github-actions bot added the stale label Oct 3, 2024
@cmester0
Copy link
Contributor Author

Closed in favor of #987 and annotated core lib.

@cmester0 cmester0 closed this Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants