Skip to content

Refactor FLINT randstate#1964

Merged
albinahlback merged 1 commit intoflintlib:mainfrom
albinahlback:declude_gmp
May 13, 2024
Merged

Refactor FLINT randstate#1964
albinahlback merged 1 commit intoflintlib:mainfrom
albinahlback:declude_gmp

Conversation

@albinahlback
Copy link
Collaborator

  • Rename flint_rand_s to flint_rand_struct,

  • Replace fields gmp_state and gmp_init in flint_rand_s by void * __gmp_state,

  • Rename the following functions:

    flint_randinit     -> flint_rand_init      (different content)
    flint_randclear    -> flint_rand_clear     (different content)
    flint_randseed     -> flint_rand_set_seed
    flint_get_randseed -> flint_rand_get_seed
    
  • Remove _flint_rand_init_gmp and replace with the two new functions _flint_rand_init_gmp_state and _flint_rand_clear_gmp_state.

    NOTE: The new init-function for the GMP-state assumes that __gmp_state is not initialized, and the new clear-function assumes that __gmp_state is initialized. To help with this, a new macro FLINT_RAND_GMP_STATE_IS_INITIALISED(state) can be utilized to check whether the GMP-state is initialized in state, assuming that state has been initialized via flint_rand_init.

  • Typedef ulong and slong to proper primitive C type instead of relying on GMP's mp_limb_t and mp_limb_signed_t.

@fredrik-johansson
Copy link
Collaborator

There should be macros for flint_randinit and flint_randclear for backward compatibility.

@albinahlback
Copy link
Collaborator Author

There should be macros for flint_randinit and flint_randclear for backward compatibility.

Yes, agreed!

@albinahlback
Copy link
Collaborator Author

I should also mention that I added clears of the random state in two embed.c files for finite fields. Looks like they haven't shown up on Valgrind yet, but they cause leaks.

@fredrik-johansson
Copy link
Collaborator

This looks fine to me. The missing underscore in flint_rand_init has been annoying for a long time.

* Rename flint_rand_s to flint_rand_struct,

* Replace fields `gmp_state` and `gmp_init` in `flint_rand_s` by
  `void * __gmp_state`,

* Rename the following functions:
  flint_randinit     -> flint_rand_init      (different content)
  flint_randclear    -> flint_rand_clear     (different content)
  flint_randseed     -> flint_rand_set_seed
  flint_get_randseed -> flint_rand_get_seed

  but keep the old names, marked as deprecated with the new keyword
  FLINT_DEPRECATED, to preserve backwards-compatibility.

* Deprecate _flint_rand_init_gmp and replace with the two new functions
  _flint_rand_init_gmp_state and _flint_rand_clear_gmp_state.

  NOTE: The new init-function for the GMP-state assumes that __gmp_state
  *is not* initialized, and the new clear-function assumes that
  __gmp_state *is* initialized. To help with this, a new macro
  `FLINT_RAND_GMP_STATE_IS_INITIALISED(state)` can be utilized to check
  whether the GMP-state is initialized in `state`, assuming that `state`
  has been initialized via `flint_rand_init`.

* Deprecate flint_rand_alloc and flint_rand_free.

* Typedef `ulong` and `slong` to proper primitive C type instead of
  relying on GMP's `mp_limb_t` and `mp_limb_signed_t`.
@albinahlback
Copy link
Collaborator Author

Nemo still needs a fix, but I will push that soon.

@albinahlback albinahlback merged commit 5166eb3 into flintlib:main May 13, 2024
@albinahlback albinahlback deleted the declude_gmp branch May 13, 2024 08:17
@fredrik-johansson
Copy link
Collaborator

CI fails on Cygwin.

@albinahlback
Copy link
Collaborator Author

Yes, I saw that. Will fix.

@user202729
Copy link
Contributor

user202729 commented Jan 31, 2025

Note that the struct rename breaks backwards compatibility sagemath/sage#39413 .

If backwards compatibility is supported for flint_rand_init maybe it should also be supported for flint_rand_s? Or is the name of that struct not supported to be in the public API?

p/s this might have to do with cython/cython#1984

@albinahlback
Copy link
Collaborator Author

Note that the struct rename breaks backwards compatibility sagemath/sage#39413 .

If backwards compatibility is supported for flint_rand_init maybe it should also be supported for flint_rand_s? Or is the name of that struct not supported to be in the public API?

p/s this might have to do with cython/cython#1984

This is a breaking change. To mitigate this, you can use preprocessor stuff to maintain compatibility with older versions.

@tornaria
Copy link
Contributor

This is a breaking change. To mitigate this, you can use preprocessor stuff to maintain compatibility with older versions.

Sure. However, since the only public use of flint_rand_s afaict was in the prototypes of flint_rand_alloc() and flint_rand_free(), which are deprecated, maybe it's reasonable that you keep those prototypes using flint_rand_s for the time being?

If you eliminate flint_rand_s you may as well just remove those functions instead of deprecating, it seems to be equivalent.

@albinahlback
Copy link
Collaborator Author

Sure. However, since the only public use of flint_rand_s afaict was in the prototypes of flint_rand_alloc() and flint_rand_free(), which are deprecated, maybe it's reasonable that you keep those prototypes using flint_rand_s for the time being?

Sorry, but I will not bring back flint_rand_s.

If you eliminate flint_rand_s you may as well just remove those functions instead of deprecating, it seems to be equivalent.

Seems reasonable.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants