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

Add C implementation; separate and package Python and C implementations #3

Merged
merged 4 commits into from
Sep 25, 2024

Conversation

AltGr
Copy link
Contributor

@AltGr AltGr commented Sep 11, 2024

TODO: add tests for the C implementation! Currently there are none.

The C implementation follows closely the OCaml one ; all functions are in reference-passing and reference-returning style, with the return value used to denote errors on functions that may fail.

OCaml, Python and C implementations have been split in lib_ocaml/, lib_python/, lib_c/ subdirectories.

The dune build specifications are extended to handle installation of the C artifacts and of the Python package content below <prefix>/lib/dates_calc/.

@AltGr AltGr requested a review from rmonat September 11, 2024 09:38
@AltGr
Copy link
Contributor Author

AltGr commented Sep 11, 2024

@rmonat I'd be glad if you could have a quick look to check the implementation vs. the formal semantics at some point, but I'm in no hurry.

@denismerigoux denismerigoux requested a review from R1kM September 11, 2024 16:57
Copy link
Collaborator

@rmonat rmonat left a comment

Choose a reason for hiding this comment

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

Thank you @AltGr! It seems really nice, I have a few minor comments.

}

/* Precondition: [1 <= d->month <= 12]. The returned day is always [1] */
void dc_add_months(dc_date *ret, const dc_date *d, const long int months) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In OCaml add_months_to_first_of_month_date is recursive, I think it is missing here? (ie when |months| >= 12)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, indeed, I changed how this function works a bit and that strays away from what's been proved.
This implementation uses a direct arithmetic computation that should be equivalent to the recursive definition.
And it was actually wrong because it used modulo computation on months in range [1, 12] ; hopefully just shifting by -1 during the computation should ensure a valid date is returned.

@rmonat
Copy link
Collaborator

rmonat commented Sep 13, 2024

TODO: add tests for the C implementation! Currently there are none.

On that note: I think you could do something similar to my Python implementation. The testcases are just defined in CSV files to ensure portability :)

@rmonat
Copy link
Collaborator

rmonat commented Sep 13, 2024

I'm not sure it's worth the extra hassle, but we may want to just have a C implementation, and then OCaml/Python modules referring to this one...

@AltGr
Copy link
Contributor Author

AltGr commented Sep 25, 2024

I took your remarks into account and implemented the CSV tests, with a couple fixes along the way (an off-by-one condition leading to a stack overflow, and a mistake on the sscanf interfaces). This should now be reasonably reliable.

I am not sure about this specific case, but regarding OCaml it's often convenient to have a pure implementation that doesn't require linking C code. Also, here I am afraid that, if we go the classical way of binding C functions in OCaml at least, writing the binding will be more work than the OCaml implementation (with the added risk of segfaults).

@rmonat
Copy link
Collaborator

rmonat commented Sep 25, 2024

Great, thanks a lot @AltGr! Feel free to do a new opam release if you require it for Catala

@rmonat rmonat merged commit 9125d78 into CatalaLang:main Sep 25, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants