search

Nemocas / Nemo.jl

Julia bindings for various mathematical libraries (including flint2)
http://nemocas.github.io/Nemo.jl/
Other
187 stars 58 forks source link

Optimize is_unit/is_zero/is_one for ZZRingElem #1808

Closed fingolfin closed 3 months ago

fingolfin commented 3 months ago

... and also is_zero_entry for ZZMatrix and FpMatrix.

Moreover, low-level accessors for ZZRingElem now also work for Ref{ZZRingElem} in addition to Ptr{ZZRingElem} and ZZRingElem.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.96%. Comparing base (79c9770) to head (e8811e5).

Files Patch % Lines
src/flint/fmpz.jl 83.33% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1808 +/- ## ========================================== + Coverage 85.95% 85.96% +0.01% ========================================== Files 98 98 Lines 36400 36401 +1 ========================================== + Hits 31286 31293 +7 + Misses 5114 5108 -6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

thofma commented 3 months ago

For the added method

is_unit(a::ZZRingElem) = data(a) == 1 || data(a) == -1

did you on purpose not add GC.@preserve a?

fingolfin commented 3 months ago

For the added method

is_unit(a::ZZRingElem) = data(a) == 1 || data(a) == -1

did you on purpose not add GC.@preserve a?

Yes. Why should it be necessary?

lgoettgens commented 3 months ago

Needs a rebase, ZZRingElemOrPtr exists twice

fingolfin commented 3 months ago

Rebased.

@thofma my question was genuine: while I don't see why the new is_zero / is_one / is_unit methods might need GC.@preserve. My reasoning is this: we just read the content of the struct via Julia code, into an Int -- afterwards this is just an Int and we can do with it whatever we want, and it doesn't matter if Julia GC's the ZZRingElem we got the value from in the meantime. So no need for GC.@preserver.

But at the same time I fully realize that I may be missing something, and I would appreciate if you share your concerns if you still have any!

And while at it, perhaps a good time to also ping @benlorenz in case he has 5 minutes to look at this.

thofma commented 3 months ago

So once we do a = x.d, in these three functions here whatever we do with the a is independent from whether x is garbage collected or not, right? Just wanted to double check.