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

making argument names in non inline functions to match documentation #1895

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

edgarcosta
Copy link
Member

this time I was more carefully with my code and looked carefully at the diff

@albinahlback
Copy link
Collaborator

Looks good! I found some minor things that we may want to change in the documentation (like padic_mat.h changed a context variable from ctx to p), but nothing major.

Are the scripts you used available for future use?

@edgarcosta
Copy link
Member Author

edgarcosta commented Mar 26, 2024

I am still working on them, I have sent you a DM on zulip with a work in progress notebook.

I think the way I generated, these changes is not very stable at the moment but could be redone, as I basically generated the file, ran it, and checked I only changed names of variables afterward.

In the previous failed PR for example, I inadvertly replaced the qualifiers, which messed up everything, due to:

# git grep fmpq_equal_fmpz
doc/source/fmpq.rst:              int fmpq_equal_fmpz(const fmpq_t x, const fmpz_t y)
src/fmpq.h:int fmpq_equal_fmpz(fmpq_t q, fmpz_t n);
src/fmpq/inlines.c:int fmpq_equal_fmpz(fmpq_t q, fmpz_t n)

@albinahlback
Copy link
Collaborator

Sorry, I forgot if we spoke about this on the workshop, but is it possible to generate a CI that checks this every time (so that we do not have regressions here)?

@edgarcosta
Copy link
Member Author

Yes, I want to have a CI that checks we don't reverse back, but it will be quite risky to have a CI that tries to fix things on the fly.
To have that CI in place, I still need to fix the inline statements. Those are a bit harder, as I will need to replace the variable in the function, and I didn't get the the time to get it working

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants