Bugs/fixes #12

[davidpfister]

This PR fixes #12 . The %tmp component has been removed and allocatable property is used instead. By doing so, the unmanaged memory is realized when the final is called.

In addition a bug in the unit test has been fixed (see dense matrix testing)
A test of the eval function has been added

And I took the liberty to create the dbl function (in symengine_basic) together with the conversion module that takes the Basic object and returns a double precision (taken from stdlib)

The code was compiled with fpm using gfortran and ifort on Windows + I checked for memory leaks using Intel Inspector which returned 'No Problems Detected' when running the unit tests.

[davidpfister]

Just for fun I also tried to compile with lfortran but it seems that it does not know yet ieee_arithmetic

[davidpfister]

I forgot to update the makefiles in my first version of the PR. It compiled just fine with fpm, and I did not see that the explicit list of files was required. I committed the fix.

[davidpfister]

I am puzzled about the last compilation issue because I get the exact opposite warning locally

I reverted my changes though

[davidpfister]

The C interop with the type c_long causes some issues. In 32-bit, c_long = 4, while in 64-bit, c_long = 8.
Now, everything should be consistent with 64-bit compilation, knowing that the 32-bit compilation will fail.

[rikardn]

Is this because of the kind needed for the literal numbers? Would it be possible to use a named kind for the literals in that case? Like 1_c_long for example. I guess we don't want the C-types to leak outside the API though.

[davidpfister]

It seems that there is an issue with you macOS environment.
Regarding your suggestion, I agree with you that the c-types should remain hidden.
We can maybe remove the first parameter (53_4 or 53_8) from the evalf fortran API, and select the correct parameter based on the size of c_long. It would give something like this

function evalf(b, r) result(res)
    class(basic), intent(in) :: b
    integer(c_int) :: r
    type(basic), allocatable :: res
    integer(c_long) :: exception
    allocate(res)
    res = Basic()
    exception = c_basic_evalf(res%ptr, b%ptr, 53_c_long, r)
    call handle_exception(exception)
end function

Amd for the dense_matrix

function zeros(r, c) result(res)
    integer(kind=int32) :: r, c
    type(DenseMatrix), allocatable :: res
    integer(c_long) :: exception
    allocate(res)
    res%ptr = c_dense_matrix_new()
    exception = c_dense_matrix_zeros(res%ptr, int(r, c_long), int(c, c_long))
    call handle_exception(exception)
end function

[davidpfister]

It looks like you CI on MacOS is broken.
I seems that the build environment is incomplete:

Error: CMAKE_Fortran_COMPILER not set, after EnableLanguage

I am no expert in GitHub actions and I do not have a mac to test it locally. Unfortunately, I will not be able to fix it.

[rikardn]

Thanks. Using gfortran on github actions on MacOS has always been a bit harder than on the other platforms. I think it is safe to ignore that fail in the context of this PR.

[davidpfister]

OK, and what about using this action?

[rikardn]

Sure, if it works!

[lkeegan]

I was actually just looking into updating this CI: #14

[davidpfister]

Cool. With the new CI it went through!
@lkeegan, well done 👍

[certik]

I think this is good. Big question is: allocatable derived type is slower than non-allocatable one. Why do we need it?

Before we used tmp. How did it work exactly? The allocatable approach in this PR seems cleaner. But is there a way to take this PR and remove allocatable? Will it not call the "finalizer" unless it is allocatable?

For example, instead of:

function pi()
    type(basic), allocatable :: pi
    allocate(pi)
    pi = Basic()
    call c_basic_const_pi(pi%ptr)
end function

why cannot we have:

function pi()
    type(basic) :: pi
    pi = Basic()
    call c_basic_const_pi(pi%ptr)
end function

?

[davidpfister]

🤔 , good point. I was under the wrong impression that the finalizer would only get called for allocatable variables. I just did the test, and the code works perfectly fine without declaring the variables allocatable. I also checked for memory leaks with intel inspector and did not find any problem.
As for the usage of the tmp variable, I can only say that it never worked for me (i.e. with ifort 2022). The compiler was complaining that the rhs in the assignment function was being modified. AFAIW, the rhs has to be declared as intent(in), and therefore it cannot be altered. I tend to agree with the diagnosis, hence the use of the final procedure.

subroutine basic_assign(a, b)
    class(basic), intent(inout) :: a
    class(basic), intent(in) :: b
    integer(c_long) :: exception
    if (.not. c_associated(a%ptr)) then
        a%ptr = c_basic_new_heap()
    end if
    exception = c_basic_assign(a%ptr, b%ptr)
    call handle_exception(exception)
    if (b%tmp) then
        call basic_free(b)
    end if
end subroutine

[certik]

I think that this looks great now! I agree, tmp feels hackish, and the finalizer approach looks clean. Now without the allocatable attribute this looks perfect.

[certik]

@davidpfister thank you. Great job.