Skip to content

Add unsafe Setters #491

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

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Add unsafe Setters #491

wants to merge 5 commits into from

Conversation

jnsiemer
Copy link
Member

Description
This PR implements...

  • public unsafe set_<fmpz_struct_name> for every underlying fmpz struct.

Testing

  • I added basic working examples (possibly in doc-comment)

Checklist:

  • I have performed a self-review of my own code
    • The code provides good readability and maintainability s.t. it fulfills best practices like talking code, modularity, ...
      • The chosen implementation is not more complex than it has to be
    • My code should work as intended and no side effects occur (e.g. memory leaks)
    • The doc comments fit our style guide

@jnsiemer jnsiemer added the enhancement📈 New feature label Mar 22, 2025
@jnsiemer jnsiemer self-assigned this Mar 22, 2025
Copy link
Member

@Marcel583 Marcel583 left a comment

Choose a reason for hiding this comment

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

Looks good. Just a question I have and some comment changes


#[cfg(test)]
mod test_get_fmpz {
use super::Z;

/// Checks availability of the getter for [`Z::value`]
/// Checks availability of the setter for [`Z::value`]
Copy link
Member

Choose a reason for hiding this comment

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

But you check the getter here. So the initial comment should be correct

Comment on lines +79 to +102
#[cfg(test)]
mod test_set_fmpz_mod_ctx {
use super::Modulus;
use flint_sys::{fmpz::fmpz, fmpz_mod::fmpz_mod_ctx_init};
use std::mem::MaybeUninit;

/// Checks availability of the setter for [`Modulus::modulus`]
/// and its ability to modify [`Modulus`].
#[test]
#[allow(unused_mut)]
fn availability_and_modification() {
let mut modulus = Modulus::from(3);

let mut flint_struct = MaybeUninit::uninit();
let mut flint_struct = unsafe {
fmpz_mod_ctx_init(flint_struct.as_mut_ptr(), &fmpz(2));
flint_struct.assume_init()
};

unsafe { modulus.set_fmpz_mod_ctx(flint_struct) };

assert_eq!(Modulus::from(2), modulus);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

How does it work for multiple references?
You say that the function only works if no other references exist/are used in the comment. But is there no way to rewrite the function such that it does? Or is it just too much for us to implement right now?
I tried this, but it did not work:

/// Checks if setter works for all references.

#[test]
#[allow(unused_mut)]
fn counter_works() {
    let mut modulus_1 = Modulus::from(3);
    let mut modulus_2 = Modulus::from(3);
    let mut modulus_3 = modulus_1.clone();

    let mut flint_struct = MaybeUninit::uninit();
    let mut flint_struct = unsafe {
        fmpz_mod_ctx_init(flint_struct.as_mut_ptr(), &fmpz(2));
        flint_struct.assume_init()
    };

    unsafe { modulus_1.set_fmpz_mod_ctx(flint_struct) };

    assert_eq!(Modulus::from(2), modulus_1);
    assert_eq!(Modulus::from(3), modulus_2);
    assert_eq!(Modulus::from(2), modulus_3);
}


unsafe_getter_mod!(Modulus, modulus, fmpz_mod_ctx);

impl Modulus {
/// Returns a mutable reference to the field `modulus` of type [`Modulus`].
Copy link
Member

Choose a reason for hiding this comment

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

The function returns nothing. It sets the value

Suggested change
/// Returns a mutable reference to the field `modulus` of type [`Modulus`].
/// Sets a mutable reference to the field `modulus` of type [`Modulus`] to a given `fmpz_mod_ctx`.

Comment on lines +24 to +27
/// **WARNING:** The returned struct is part of [`flint_sys`].
/// Any changes to this object are unsafe and may introduce memory leaks.
/// Please be aware that most moduli are shared across multiple instances and all
/// modifications of this struct will affect any other instance with a reference to this object.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// **WARNING:** The returned struct is part of [`flint_sys`].
/// Any changes to this object are unsafe and may introduce memory leaks.
/// Please be aware that most moduli are shared across multiple instances and all
/// modifications of this struct will affect any other instance with a reference to this object.
/// **WARNING:** The set struct is part of [`flint_sys`].
/// Any changes to this object are unsafe and may introduce memory leaks.
/// Please be aware that most moduli are shared across multiple instances and all
/// modifications of this struct will affect any other instance with a reference to this object.


unsafe_getter_mod!(ModulusPolynomialRingZq, modulus, fq_ctx_struct);

impl ModulusPolynomialRingZq {
/// Returns a mutable reference to the field `modulus` of type [`ModulusPolynomialRingZq`].
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Returns a mutable reference to the field `modulus` of type [`ModulusPolynomialRingZq`].
/// Sets a mutable reference to the field `modulus` of type [`ModulusPolynomialRingZq`] to a given `fq_ctx_struct`.

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

Successfully merging this pull request may close these issues.

2 participants