Result type int #36
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ module m_result_dp_manager | |
| use kind_parameters, only: dp | ||
| use m_error_v, only: ErrorV | ||
| use m_result_dp, only: ResultDP | ||
| use m_result_int, only: ResultInt | ||
| use m_result_none, only: ResultNone | ||
|
|
||
| implicit none(type, external) | ||
|
|
@@ -18,7 +19,7 @@ module m_result_dp_manager | |
|
|
||
| contains | ||
|
|
||
| function build_instance(data_v_in, error_v_in) result(res_available_instance_index) | ||
| !! Build an instance | ||
|
|
||
| real(kind=dp), intent(in), optional :: data_v_in | ||
|
|
@@ -27,14 +28,18 @@ function build_instance(data_v_in, error_v_in) result(instance_index) | |
| class(ErrorV), intent(in), optional :: error_v_in | ||
| !! Error message | ||
|
|
||
| type(ResultInt) :: res_available_instance_index | ||
| !! Index of the built instance | ||
|
|
||
| type(ResultNone) :: res_build | ||
|
|
||
| call ensure_instance_array_size_is_at_least(1) | ||
| call get_available_instance_index(res_available_instance_index) | ||
| ! MZ check for errors ? | ||
| ! MZ function with side effect: good idea?? | ||
|
Contributor
There was a problem hiding this comment. ye I don't like it either, see my comments on the function |
||
| ! MZ why res_build is ResultNone?? | ||
|
Contributor
There was a problem hiding this comment. we're calling a method on the object we already have so I was following the python convention of returning none which means the result has to be ResultNone. Is there another option? |
||
| res_build = instance_array(res_available_instance_index%data_v) % & | ||
| build(data_v_in=data_v_in, error_v_in=error_v_in) | ||
|
|
||
| ! TODO: check build has no error | ||
|
|
||
|
|
@@ -46,44 +51,46 @@ subroutine finalise_instance(instance_index) | |
| integer, intent(in) :: instance_index | ||
| !! Index of the instance to finalise | ||
|
|
||
| type(ResultNone) :: res_check_index_claimed | ||
|
|
||
| res_check_index_claimed = check_index_claimed(instance_index) | ||
| if(res_check_index_claimed%is_error()) return | ||
|
|
||
| call instance_array(instance_index) % finalise() | ||
| instance_available(instance_index) = .true. | ||
|
|
||
| end subroutine finalise_instance | ||
|
|
||
| subroutine get_available_instance_index(res_available_instance_index) | ||
| !! Get a free instance index | ||
|
|
||
| ! TODO: think through whether race conditions are possible | ||
| ! e.g. while returning a free index number to one Python call | ||
| ! a different one can be looking up a free instance index at the same time | ||
| ! and something goes wrong (maybe we need a lock) | ||
|
|
||
| type(ResultInt), intent(out) :: res_available_instance_index | ||
| ! integer, intent(out) :: available_instance_index | ||
| !! Available instance index | ||
| integer :: i | ||
|
|
||
| do i = 1, size(instance_array) | ||
|
|
||
| if (instance_available(i)) then | ||
|
|
||
| instance_available(i) = .false. | ||
| ! available_instance_index = i | ||
| ! TODO: switch to returning a Result type | ||
| res_available_instance_index = ResultInt(data_v=i) | ||
| return | ||
|
|
||
| end if | ||
|
|
||
| end do | ||
|
|
||
| ! TODO: switch to returning a Result type with an error set | ||
| res_available_instance_index = ResultInt(error_v=ErrorV(code=1, message="No available instances")) | ||
| ! error stop 1 | ||
| end subroutine get_available_instance_index | ||
|
|
||
| ! Change to pure function when we update check_index_claimed to be pure | ||
|
|
@@ -95,8 +102,14 @@ function get_instance(instance_index) result(inst) | |
| type(ResultDP) :: inst | ||
| !! Instance at `instance_array(instance_index)` | ||
|
|
||
| type(ResultNone) :: res_check_index_claimed | ||
|
|
||
| res_check_index_claimed = check_index_claimed(instance_index) | ||
| if(res_check_index_claimed%is_error()) then | ||
| inst = ResultDP(error_v=res_check_index_claimed%error_v) | ||
| else | ||
| inst = instance_array(instance_index) | ||
| end if | ||
|
|
||
| end function get_instance | ||
|
|
||
|
|
@@ -106,19 +119,22 @@ subroutine set_instance_index_to(instance_index, val) | |
| !! Index in `instance_array` of which to set the value equal to `val` | ||
|
|
||
| type(ResultDP), intent(in) :: val | ||
| type(ResultNone) :: res_check_index_claimed | ||
|
|
||
| res_check_index_claimed = check_index_claimed(instance_index) | ||
| if(res_check_index_claimed%is_error()) instance_array(instance_index) = val | ||
|
|
||
| end subroutine set_instance_index_to | ||
|
|
||
| function check_index_claimed(instance_index) result(res_check_index_claimed) | ||
| !! Check that an index has already been claimed | ||
| !! | ||
| !! Stops execution if the index has not been claimed. | ||
|
|
||
| integer, intent(in) :: instance_index | ||
| !! Instance index to check | ||
| type(ResultNone) :: res_check_index_claimed | ||
| character(len=:), allocatable :: msg | ||
|
|
||
| if (instance_available(instance_index)) then | ||
| ! TODO: Switch to using Result here | ||
|
|
@@ -128,8 +144,12 @@ subroutine check_index_claimed(instance_index) | |
| ! if it fails, the result_dp attribute will be set). | ||
| ! So the code would be something like | ||
| ! res = ResultNone(ResultDP(code=1, message="Index ", instance_index, " has not been claimed")) | ||
| ! print *, "Index ", instance_index, " has not been claimed" | ||
| ! error stop 1 | ||
| ! MZ Weird thing allocatable message | ||
|
Contributor
There was a problem hiding this comment. yep lots of mucking around. We may end up writing something like this https://gitlab.com/magicc/fgen/-/blob/switch-to-uv/packages/fgen-core/src/char_conversions.f90?ref_type=heads or using someone else's equivalent if we can find a nice one
Contributor
There was a problem hiding this comment. ok the pros are here: https://github.com/fortran-lang/stdlib we would either use their functions or could also just look at them and write our own versions because a) stdlib is MIT licensed so it's fine and b) we don't introduce the entire stdlib as a dependency that way |
||
| msg = "" | ||
|
|
||
| write(msg,fmt="(A, I0, A)") "Index ", instance_index," has not been claimed" | ||
| res_check_index_claimed = ResultNone(error_v=ErrorV(code=1, message=msg)) | ||
| end if | ||
|
|
||
| if (instance_index < 1) then | ||
|
|
@@ -140,8 +160,10 @@ subroutine check_index_claimed(instance_index) | |
| ! if it fails, the result_dp attribute will be set). | ||
| ! So the code would be something like | ||
| ! res = ResultNone(ResultDP(code=2, message="Requested index is ", instance_index, " which is less than 1")) | ||
| ! print *, "Requested index is ", instance_index, " which is less than 1" | ||
| ! error stop 1 | ||
| write(msg,fmt="(A, I0, A)") "Requested index is ", instance_index, " which is less than 1" | ||
| res_check_index_claimed = ResultNone(error_v=ErrorV(code=2, message=msg)) | ||
| end if | ||
|
|
||
| ! ! Here, result becomes | ||
|
|
@@ -151,8 +173,9 @@ subroutine check_index_claimed(instance_index) | |
| ! ! We will no longer have subroutines that return nothing | ||
| ! ! (like this one currently does). | ||
| ! res = ResultNone() | ||
| res_check_index_claimed = ResultNone() | ||
|
|
||
| end function check_index_claimed | ||
|
|
||
| subroutine ensure_instance_array_size_is_at_least(n) | ||
| !! Ensure that `instance_array` and `instance_available` have at least `n` slots | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep we want to switch this to returniing ResultInt