remove unused Base.GMP.MPZ.import! and test Base.GMP.MPZ.export! #56903

NegaScout · 2024-12-24T16:01:01Z

rename Base.GMP.MPZ.export! "dynamic" variable from a to x, since it is more in line with comment on L141 in base/gmp.jl

julia/base/gmp.jl

Lines 141 to 146 in cab11bb

    
           # wrapping of libgmp functions 
        
           # - "output parameters" are labeled x, y, z, and are returned when appropriate 
        
           # - constant input parameters are labeled a, b, c 
        
           # - a method modifying its input has a "!" appended to its name, according to Julia's conventions 
        
           # - some convenient methods are added (in addition to the pure MPZ ones), e.g. `add(a, b) = add!(BigInt(), a, b)` 
        
           #   and `add!(x, a) = add!(x, x, a)`.

add tests, that numbers and vectors can be exported and imported correctly and composed import and export is identity

LilithHafner · 2024-12-24T20:02:17Z

Thanks!

AFAICT, import! is an internal function that is neither used nor tested. Perhaps it should be removed? As of 6d71254, this PR makes a breaking change to import! so if we run pkgeval we should be able to tell if anyone depends on this internal function. export! is used once and untested so adding tests for it is very nice.

This invokes pkgeval: @nanosoldier runtests()

NegaScout · 2024-12-24T20:54:00Z

Oh, I wanted to run some kind of grep at least, if it is used somewhere, but I forgot. Sorry. However, this functionality is very handy for "interpreting memory as number". I am not sure if there is more use cases. However it would be nice for any serious RSA implementation (and ElGamal at least I think, possibly more cryptographic schemes), which needs to interpret memory as a number to sign it or encrypt/decrypt it.

I would like to see movement in the opposite direction and would like to see more support for this. Otherwise, some iteration based construction for this use case would be needed, unless some Julia native BigInt type is present in Base.

I also think, any big number serialization/deserialization would benefit from this.

However, I can implement any proposed changes or feel free to close this, if it is not desired change

giordano · 2024-12-24T22:37:18Z

No uses outside of Base according to https://juliahub.com/ui/Search?type=symbols&q=import!

nanosoldier · 2024-12-26T02:16:08Z

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

LilithHafner · 2024-12-26T13:47:19Z

Generally, I lean toward avoiding new public API unless there is a very compelling case. The fact that nobody has used this internal function all this time makes me doubt that it is valuable commonly enough to warrant including in the public API. For those reasons, I prefer deleting insert! rather than publicizing it.

That said, it would be nice to hear other folks' opinions on whether we should keep/publicize/remove Base.GMP.MPZ.import!.

Let's talk about it at the next triage meeting Thursday at 1:15 ET.

LilithHafner · 2025-01-02T18:36:26Z

Triage talked about this and considered weather we want to provide a user facing wrapper of MPZ or if we just want to keep these internal and use only what we need to implement BigInts and other Base functions. Making a good user-facing wrapper of MPZ would be fairly large and we don't want to put that in Base. Instead, it makes more sense for a fully featured MPZ wrapper to exist as a package (open invitation to anyone who wants to create that package :) ). Making import! public without also making many more functions public wouldn't really make sense.

For import!, specifically, it makes sense to delete the function from Base because we're not using it and don't want to add it to the public API.

NegaScout · 2025-01-02T18:58:13Z

Ok, fair. It makes sense since there is still much in MPZ that would be cool to have in Julia but does not have bindings. So, lets rebase it and keep only the test and argument rename?

LilithHafner · 2025-01-02T19:55:59Z

It would also be good to move the definition of import! from base/ to test/

NegaScout · 2025-01-11T14:15:14Z

Not sure, if this is how you meant to move the definition from base to tests. However I could not define Base_GMP_MPZ_import as Base.GMP.MPZ.import! in the test, not sure why 🤔

LilithHafner

Not sure, if this is how you meant to move the definition from base to tests.

Yes, that's it exactly. It would be bad to define Base.GMP.MPZ.import! in tests because then someone could write tests for that function that appear to be testing a function defined in Base when that function actually does not exist in Base julia!

LilithHafner · 2025-01-12T11:11:31Z

base/gmp.jl

+function export!(x::AbstractVector{T}, n::BigInt; order::Integer=-1, nails::Integer=0, endian::Integer=0) where {T<:Base.BitInteger}
+    stride(x, 1) == 1 || throw(ArgumentError("x must have stride 1"))
    ndigits = cld(sizeinbase(n, 2), 8*sizeof(T) - nails)
-    length(a) < ndigits && resize!(a, ndigits)
+    length(x) < ndigits && resize!(x, ndigits)
    count = Ref{Csize_t}()
-    ccall((:__gmpz_export, libgmp), Ptr{T}, (Ptr{T}, Ref{Csize_t}, Cint, Csize_t, Cint, Csize_t, mpz_t),
-        a, count, order, sizeof(T), endian, nails, n)
-    @assert count[] ≤ length(a)
-    return a, Int(count[])
+    ccall((:__gmpz_export, libgmp),
+           Ptr{T},
+           (Ptr{T}, Ref{Csize_t}, Cint, Csize_t, Cint, Csize_t, mpz_t),
+           x, count, order, sizeof(T), endian, nails, n)
+    @assert count[] ≤ length(x)
+    return x, Int(count[])
 end


We try to avoid committing formatting-only changes unless they are meaningful improvements and/or the old format was clearly bad. In this case, renaming this vector from a to x and changing line breaks just doesn't seem worth doing.

OTOH, I really don't care that much about this formatting so if you want to leave these changes in that's okay with me.

I ended up removing it. It was either remove, or do separate commit and at that point it did not make sense anymore I think

LilithHafner · 2025-01-12T11:12:20Z

test/gmp.jl

@@ -445,6 +445,44 @@ end
    @test string(big(0), base = rand(2:62), pad = 0) == ""
 end

+@testset "Base.GMP.MPZ.import!/export!" begin


Suggested change

@testset "Base.GMP.MPZ.import!/export!" begin

@testset "Base.GMP.MPZ.export!" begin

At this point, the testset is only testing export! because import! isn't a thing anymore (outside of the testset itself)

add tests, that numbers and vectors can be exported and imported correctly and composed import and export is identity

LilithHafner · 2025-01-15T13:13:45Z

Thanks, @NegaScout!

giordano added the bignums BigInt and BigFloat label Dec 24, 2024

LilithHafner added the triage This should be discussed on a triage call label Dec 26, 2024

LilithHafner removed the triage This should be discussed on a triage call label Jan 2, 2025

NegaScout force-pushed the jw/import_export branch from 6d71254 to c1c49cc Compare January 11, 2025 14:13

LilithHafner added test This change adds or pertains to unit tests excision Removal of code from Base or the repository labels Jan 12, 2025

LilithHafner approved these changes Jan 12, 2025

View reviewed changes

remove refactor Base.GMP.MPZ.import! from base

dca8c16

add tests, that numbers and vectors can be exported and imported correctly and composed import and export is identity

NegaScout force-pushed the jw/import_export branch from c1c49cc to dca8c16 Compare January 14, 2025 22:11

LilithHafner changed the title ~~refactor Base.GMP.MPZ.import! to be consistent with Base.GMP.MPZ.export!~~ remove unused Base.GMP.MPZ.import! and test Base.GMP.MPZ.export! Jan 15, 2025

LilithHafner merged commit 6cf2b14 into JuliaLang:master Jan 15, 2025
6 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove unused Base.GMP.MPZ.import! and test Base.GMP.MPZ.export! #56903

remove unused Base.GMP.MPZ.import! and test Base.GMP.MPZ.export! #56903

NegaScout commented Dec 24, 2024 •

edited by giordano

Loading

LilithHafner commented Dec 24, 2024

NegaScout commented Dec 24, 2024 •

edited

Loading

giordano commented Dec 24, 2024 •

edited

Loading

nanosoldier commented Dec 26, 2024

LilithHafner commented Dec 26, 2024 •

edited

Loading

LilithHafner commented Jan 2, 2025

NegaScout commented Jan 2, 2025 •

edited

Loading

LilithHafner commented Jan 2, 2025

NegaScout commented Jan 11, 2025

LilithHafner left a comment

LilithHafner Jan 12, 2025

NegaScout Jan 14, 2025

LilithHafner Jan 12, 2025

LilithHafner commented Jan 15, 2025

	# wrapping of libgmp functions
	# - "output parameters" are labeled x, y, z, and are returned when appropriate
	# - constant input parameters are labeled a, b, c
	# - a method modifying its input has a "!" appended to its name, according to Julia's conventions
	# - some convenient methods are added (in addition to the pure MPZ ones), e.g. `add(a, b) = add!(BigInt(), a, b)`
	# and `add!(x, a) = add!(x, x, a)`.

	@testset "Base.GMP.MPZ.import!/export!" begin
	@testset "Base.GMP.MPZ.export!" begin

remove unused Base.GMP.MPZ.import! and test Base.GMP.MPZ.export! #56903

remove unused Base.GMP.MPZ.import! and test Base.GMP.MPZ.export! #56903

Conversation

NegaScout commented Dec 24, 2024 • edited by giordano Loading

LilithHafner commented Dec 24, 2024

NegaScout commented Dec 24, 2024 • edited Loading

giordano commented Dec 24, 2024 • edited Loading

nanosoldier commented Dec 26, 2024

LilithHafner commented Dec 26, 2024 • edited Loading

LilithHafner commented Jan 2, 2025

NegaScout commented Jan 2, 2025 • edited Loading

LilithHafner commented Jan 2, 2025

NegaScout commented Jan 11, 2025

LilithHafner left a comment

Choose a reason for hiding this comment

LilithHafner Jan 12, 2025

Choose a reason for hiding this comment

NegaScout Jan 14, 2025

Choose a reason for hiding this comment

LilithHafner Jan 12, 2025

Choose a reason for hiding this comment

LilithHafner commented Jan 15, 2025

NegaScout commented Dec 24, 2024 •

edited by giordano

Loading

NegaScout commented Dec 24, 2024 •

edited

Loading

giordano commented Dec 24, 2024 •

edited

Loading

LilithHafner commented Dec 26, 2024 •

edited

Loading

NegaScout commented Jan 2, 2025 •

edited

Loading