refactor malloc (cherry pick from fm/task/UNST-9773_meteo_reader)#761
refactor malloc (cherry pick from fm/task/UNST-9773_meteo_reader)#761FlorisBuwaldaDeltares wants to merge 7 commits intomainfrom
Conversation
FlorisBuwaldaDeltares
requested review from
KoenBussemakerDeltares and
harmenwierenga
There was a problem hiding this comment.
Did you see src\test\utils_lgpl\deltares_common\packages\test_deltares_common\src\test_malloc.f90? It has the FTNunit tests that I added in my first month. It's a part of https://issuetracker.deltares.nl/browse/UNST-8998 to port these to F90TW
| subroutine test_realloc_same_bounds_no_keepexisting_fill() bind(C) | ||
| real(dp), allocatable, target :: arr(:) | ||
| real(dp), pointer :: ptr_before | ||
| call realloc(arr, 3, fill=1.0d0) |
There was a problem hiding this comment.
I am seeing 1.0d0. Can you run the fortran_styler on this file?
| call realloc(arr, 5, fill=-999.0d0) | ||
| call f90_expect_true(allocated(arr)) | ||
| call f90_expect_eq(size(arr), 5) | ||
| call f90_expect_true(all(arr == -999.0d0)) |
There was a problem hiding this comment.
Please check if they are close, floating point comparisons can be really unstable
| subroutine test_realloc_grow_keeps_existing() bind(C) | ||
| real(dp), allocatable :: arr(:) | ||
| call realloc(arr, 3, fill=0.0d0) | ||
| arr(1) = 1.0d0; arr(2) = 2.0d0; arr(3) = 3.0d0 |
There was a problem hiding this comment.
I see semicolons, you can run the fortran_styler
| ptr_before => arr(1) | ||
| call realloc(arr, 3, fill=-999.0d0, keepExisting=.false.) | ||
| call f90_expect_true(all(arr == -999.0d0)) | ||
| call f90_expect_true(associated(ptr_before, arr(1))) |
There was a problem hiding this comment.
That's nice, my first thought would be to compare the loc(arr) directly, but this is neat as well and perhaps even more meaningful
| subroutine test_realloc_shift() bind(C) | ||
| real(dp), allocatable :: arr(:) | ||
| call realloc(arr, 3, fill=0.0d0) | ||
| arr(1) = 10.0d0; arr(2) = 20.0d0; arr(3) = 30.0d0 |
| integer, allocatable :: arr(:,:) | ||
| call realloc(arr, [3, 2], fill=0) | ||
| arr(1,1) = 1; arr(2,1) = 2; arr(3,1) = 3 | ||
| arr(1,2) = 4; arr(2,2) = 5; arr(3,2) = 6 |
There was a problem hiding this comment.
To fill these arrays, I think that it is nicer to say arr = reshape([1, 2, 3, 4, 5, 6], [3, 2]) or even reshape(iota_initialize(6), shape(arr)) (if you take iota_initialize from the other unit tests file)
| end if | ||
| end if | ||
|
|
||
| MOVE_ALLOC |
There was a problem hiding this comment.
As I mentioned, I doubt that the complexity of a macro approach weighs up against the benefit of deduplication in this case. If you really want to continue with the macro approach, then I think that a few changes are necessary:
- This whole file should be a function-like macro. The define / undefine logic makes it very error prone and hard to read, since those definitions act like globals within that file. This function-like macro should have the type, whether its a pointer, and the dimension I think. The rest seem to be implementation details
- Instead of a MOVE_ALLOC macro, I think that it is more readable to have an # if / # else macro construction with the code depending on whether it is a pointer or not. I think that IS_ALLOCATED can be a local macro that is set by an # if / # else depending on whether it is a pointer. But two functions copies instead of such an 'IS_POINTER' flag is also acceptable.
- It is a bit cleaner to have goto 999 instead of a return, to have a single error handling path. The if (present(stat)) only has to be handled once.
There was a problem hiding this comment.
I think i can do that
There was a problem hiding this comment.
The fortran preprocessor does not support if-statements inside defines, so this is not possible
There was a problem hiding this comment.
Of course. I now also realise again that these macros are a compiler extension. If we moved to a different compiler, then this may all need to be reverted.
There was a problem hiding this comment.
ok if I make two copies, one for allocatables one for pointers it's possible
| else | ||
| shift_ = (/0, 0, 0, 0/) | ||
| end if | ||
| REALLOC1(reallocDouble, real(dp)) |
There was a problem hiding this comment.
This became a lot better. Would the higher-rank array versions require a lot of change? My guess would be that at most we need two more copies to match the rank 2 to 4 stuff, or is it even possible by adding one parameter to the function-like macros?
| @@ -0,0 +1,133 @@ | |||
| #define REALLOC1(SUBNAME, DTYPE) \ | |||
There was a problem hiding this comment.
Could you add a comment explaining the subroutine? I think that SUBNAME and DNAME could be SUBROUTINE_NAME and DATA_TYPE, respectively. There is still an early return that needs to be changed to a goto.
3 participants
What was done
Evidence of the work done
<add video/figures if applicable>
Tests
<add testcase numbers if applicable, Issue number>
Documentation
<add description of changes if applicable, Issue number>
Issue link