search

nordlow / gmp-d

D-language high-level wrapper for GNU MP (GMP) library
14 stars 6 forks source link

Add mpz_export #8

Closed basiliscos closed 6 years ago

basiliscos commented 6 years ago

Here is a first attempt. Please, share your opinion.

I also will need of mpz_import, but I think about implemeinting it as a constructor

basiliscos commented 6 years ago

What about returning the same rop aswell to make the usage of convert more functional?

Hm, it seems we need 2 params then - number of written words AND the rop ?

nordlow commented 6 years ago

You slice rop into a potentially smaller array as

return rop[0 .. writeCount]

provided that writeCount <= rop.length and typeof(rop[0]) is mpz word type.

basiliscos commented 6 years ago

Okay, got it, will do (a bit later) today.

basiliscos commented 6 years ago

The version with gc-allocating slice should have signature like 

T[] convert(T)(WordOrder order, size_t size, Endianess endian, size_t nails)?

right?

nordlow commented 6 years ago

Yes, except that the size parameter is not needed since it is the word-size, which in your case is T.sizeof.

nordlow commented 6 years ago

The documentation of mpz_import is an excellent (terrible) example of faith-based programming which no language tries to mimic anymore. It's not at all easy to decode how all these parameters will be combined. I'll have to think about this for a while.

Best way to test this is to write a loop that mpz_exports and then mpz_imports a large set of numbers and then checks that you get back in the import what you sent into the export. I'd pick a set of odd numbers such as 0, +-1, +-3, +-9, +-27, etc.

basiliscos commented 6 years ago 
Error: setting length in @nogc function gmp.z._MpZ!false._MpZ.convert!ubyte.convert may cause a GC allocation

How to disablle @nogc: attribute for the 2nd convert version?

nordlow commented 6 years ago

Unfortunately there is no way disable @nogc:. Can you move the convert function up to before the definition of @nogc?

Note that @nogc: is not needed as that attribute is inferred on all the members in template structs, in our case _MpZ(bool copyable = false). But I'm keeping it here for now just to make sure that I don't accidentally make use of GC-allocations when I shouldn't.

nordlow commented 6 years ago

BTW: https://github.com/dlang/DIPs/blob/master/DIPs/DIP1012.md#examples ;)

basiliscos commented 6 years ago

Yeah, that's looks good!

Can we merge the current PR? I plan to do mpz_import as separate PR

nordlow commented 6 years ago

Done! Thanks. Ok to go ahead with import now!

nordlow commented 6 years ago

Why do you prefer the naming convert? The standard naming is 'to' defined in std.conv.

basiliscos commented 6 years ago

Why do you prefer the naming convert? The standard naming is 'to' defined in std.conv.

Well, I was'nt aware of to. Could you, please, fix it?

nordlow commented 6 years ago

Yep. Shall we call the static factory function (that wraps mpz_import) from then?

basiliscos commented 6 years ago

Okay, if from suits more. I was sure that simple constructor should be enough.

nordlow commented 6 years ago

Use a normal constructor for now, I'm not certain what's best in this case.

I asked the forums about what's the best naming in this case. We can wait and see which ideas that get the most votes at: https://forum.dlang.org/post/nsoomaqffwffgovlboma@forum.dlang.org