-
Notifications
You must be signed in to change notification settings - Fork 182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
implemented move_alloc for string_type #467
Conversation
It can't be
It can't overload use stdlib_string_type
type(string_type), allocatable :: str1, str2
str1 = string_type("Moveable string value")
call move_alloc(str1, str2)
end |
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.
This is a useful addition, thanks.
Requesting Clarification: As I understand, the function can't be made |
How about instead of overloading the character(len=:), allocatable :: char_1, char_2
char_1 = "moveable char"
move(char_1, char_2) ! char_2 <-- "moveable char"
move_alloc(char_2, char_1) ! char_1 <-- "moveable char" above expected behaviour can be carried out using |
@awvwgk I have added a commit 73ec8c5 corresponding to comment mentioned just above by me. I added |
type(string_type), intent(inout) :: from | ||
type(string_type), intent(out) :: to | ||
|
||
call move_alloc(from%raw, to%raw) |
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.
move_alloc
requires its arguments to be allocatable
.
The move
function arguments here are type(string_type)
, which is not allocatable
.
If we overload move_alloc
, it may cause users to misunderstand that move_alloc
is different from the Fortran standard at allocatable
attribute .
From this perspective, I think the name can be move
or move_alloc_
, better not move_alloc
?
But from the perspective of actual use and as an extended function, I think move_alloc
is also good.
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.
I found move
to be intuitive as well. As if it cut and pastes (not exactly) a string from one location to other. Similar to moving a file from one folder to other.
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.
Very nice, thanks @aman-godara. We'll merge it tomorrow if there are no objections.
But wait! I have objections here. Have a look at commit 73ec8c5 once. I will also improve test cases and documentation for this. |
Did I understand it correctly? I have made all the change that I wanted to make. PR is open to be merged from my side. I found the behaviour of the last test case non-intuitive but I think we cannot do anything with it. |
@aman-godara correct, the dummy argument can't be I also don't see a use case for the last test, but I can't tell that it hurts to leave it there. Let's merge tomorrow if no objections. Thanks again! |
Closes #449
Tasks:
move
move
if both arguments are of typecharacter(len=:)
?:move_alloc
does the same thing OR should we consider renaming it frommove
tomove_alloc
.