Add in_place suffix for BigInteger methods

[tcoratger]

Description

This pull request introduces a standardized naming convention for methods in the BigInteger module, specifically targeting functions where the self mutable value is modified in-place. The convention adds the in_place suffix to method names to provide clarity and consistency in the codebase.

closes: #781

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (master)
• Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
• Wrote unit tests
• Updated relevant documentation in the code
• Added a relevant changelog entry to the Pending section in CHANGELOG.md
• Re-reviewed Files changed in the GitHub PR explorer

[tcoratger]

@Pratyush I thought about this PR and because the MulAssign or AddAssign traits of the Rust library use the suffix Assign, we will necessarily have mul_assign or add_assign which will be present somewhere in the code.

To unify everything, wouldn't it be better to rename everything throughout the code with an assign suffix rather than in_place. And for example the instances of square_in_place would also become square_assign, what do you think?

[mmaker]

I'd much rather have a _assign indeed. What do people think?

[Pratyush]

Wouldn’t it be a massive breaking change?

[mmaker]

Not more than adding thein_place suffix? If it helps with naming consistency down the line before 1.0 I think that's a valid effort (and we should deprecate the previous functions, not just remove them)

[Pratyush]

Ah I assumed it was to do it for all of the other traits too, like Field and GroupOn Feb 28, 2024, at 8:51 AM, Michele Orrù ***@***.***> wrote: Not more than adding thein_place suffix? If it helps with naming consistency down the line before 1.0 I think that's a valid effort (and we should deprecate the previous names, not just remove them) —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[mmaker]

[EDIT]To phrase better my comment:

In Field we have

double_in_place (from AdditiveGroup)
neg_in_place (from AdditiveGroup)
sqrt_in_place
square_in_place
inverse_in_place
frobenius_map_in_place

I think it's nice to have either (1) in_place everywhere except trait implementations of std::ops::*, OR (2) _assign everywhere. I'd be consistent with whatever we pick, and propagate this change to ark-ff and ark-ec, deprecating old function calls that will be removed for 1.0.
I fear having intermediate solutions will make the naming conventions of the library confusing.

@Pratyush In your comment, are you implying that having _assign only for BigInt is acceptable?
@tcoratger What do you think?

[tcoratger]

@mmaker Agreed, I found it confusing at the beginning like in Montgomery backend for exemple we have:

• add_assign
• sub_assign
• double_in_place
• neg_in_place

The std library always uses the _assign suffix for all operations so for me it would be worth changing everything to have consistent naming everywhere (_assign in a std like style) in the codebase. But would agree that this is quite a massive change of a big part of the function names of the whole library.

[burdges]

"Assignment operators" is the C term, so _assign made sense for operators, and hence maybe sensible for methods resembling operator methods, ala mul2_assign.

Afaik _in_place was always more traditional overall, and std uses in_place for pointers, so nothing wrong with _in_place either, but maybe resembling operators should be the priority.

Also, big integers are bigish for high performace code, so if a non-in-place variant feels particularly rediculous for operation foo, then foo could be the in-place variant, and _cloned could be the non-in-place variant, but again maybe resembling operators should be the priority.