Skip to content

Added to_lower, to_upper, reverse and to_title function to stdlib_string_type.f90 file #346

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

Merged
merged 12 commits into from
Mar 21, 2021

Conversation

aman-godara
Copy link
Member

@aman-godara aman-godara commented Mar 15, 2021

Status: open for review.

Implemented: the functions asked in #335, added unit tests for each of these functions in test_string_functions.f90 file and documented the functions in stdlib_string_type.md file. (Fixes #335)
Yet to be done in this pull request: #346 (comment) (done in the pull request #356)
Note: Please have a look at the recent suggestion comment concerning to_title function

I am still trying to get the issues concerning my compiler fixed to run the code on my machine. The code written is not checked to run successfully on machine yet. Thus, I kindly request the reviewers to please also run the code once.

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing. I suggest to rename the imports from stdlib_ascii to avoid polluting the namespace of stdlib_string_type and reexporting them accidentally.

I recommend to use the maybe() wrapper for safe memory access inside stdlib_string_type rather than directly accessing the raw value on the right-hand-side.

Note: here how using maybe(string) automatically access the string%raw.
maybe is used for safe memory access.

Co-authored-by: Sebastian Ehlert <[email protected]>
@awvwgk
Copy link
Member

awvwgk commented Mar 15, 2021

Should the newly created functions in stdlib_string_type.f90 module (to_lower_string, to_upper_string, etc.) be elemental or pure?

Elemental should be the way to go here. For stdlib_ascii we discussed the usage of pure vs elemental briefly here: #310 (comment). Those concerns are mainly related to the intrinsic character type and not the string_type.


Also, the manual Makefile failure is related to the newly introduced dependency of stdlib_string_type on stdlib_ascii and can be fixed by adding

stdlib_string_type.o: stdlib_ascii.o

to src/Makefile.manual.

@aman-godara

This comment has been minimized.

@aman-godara
Copy link
Member Author

Can I create a new .f90 file in the src/tests/string to create test cases for the functions implemented above or should I edit an already existing file in the folder.

@awvwgk
Copy link
Member

awvwgk commented Mar 18, 2021

Can I create a new .f90 file in the src/tests/string to create test cases for the functions implemented above or should I edit an already existing file in the folder.

You can add a new file, but make sure to register the unit test in the CMake build file in the same directory than as well.

@aman-godara
Copy link
Member Author

aman-godara commented Mar 18, 2021

What happens when a user gives a string_type object on the right hand side instead of a character sequence while handling the assignment operator(=) with string_type variables. I think current implementation doesn't handle such possibility.

Copy link
Member

@milancurcic milancurcic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks @aman-godara!

@milancurcic
Copy link
Member

What happens when a user gives a string_type object on the right hand side instead of a character sequence while handling the assignment operator(=) with string_type variables. I think current implementation doesn't handle such possibility.

Assigning string_type to another string_type comes out-of-the box with the derived type. It should just work. Is that what you mean or something else?

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aman-godara Can you add the new procedures to the specification of the string_type in doc/specs/stdlib_string_type.md as well?

@aman-godara
Copy link
Member Author

aman-godara commented Mar 19, 2021

@awvwgk Yes, I am working on documenting the functions created. I just needed a little help with #346 (comment) to proceed further because I am at the starting stage of learning CMake.

@milancurcic yes, you got it right. I wanted to talk more about stdlib_string_type, but before that let me try to figure things out on my own by learning fortran.

@aman-godara aman-godara marked this pull request as ready for review March 19, 2021 10:40
Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good now.

@milancurcic
Copy link
Member

With two approvals, I will merge. Thank you, @aman-godara!

@aman-godara
Copy link
Member Author

I recommend to use the maybe() wrapper for safe memory access inside stdlib_string_type rather than directly accessing the raw value on the right-hand-side.

@awvwgk does maybe() wrapper handle the case when raw (component of string_type derived type) is not allocated?

@awvwgk
Copy link
Member

awvwgk commented Apr 4, 2021

Indeed, it does make sure that an unallocated raw component of the string_type behaves like an empty string:

!> Safely return the character sequences represented by the string
pure function maybe(string) result(maybe_string)
type(string_type), intent(in) :: string
! GCC 8 and older cannot evaluate pure derived type procedures here
!character(len=len(string)) :: maybe_string
character(len=:), allocatable :: maybe_string
if (allocated(string%raw)) then
maybe_string = string%raw
else
maybe_string = ''
end if
end function maybe

@aman-godara
Copy link
Member Author

aman-godara commented Apr 4, 2021

May I ask why we didn't design the constructor to have an empty character sequence ("") in raw when optional argument string is not given?

elemental function new_string(string) result(new)
character(len=*), intent(in), optional :: string
type(string_type) :: new
if (present(string)) then
new%raw = string
end if
end function new_string

@awvwgk
Copy link
Member

awvwgk commented Apr 4, 2021

There is still the possibility to have an uninitialized string_type which will have an unallocated raw component by default as in

use stdlib_string_type
implicit none
type(string_type) :: str
print *, str
end

@aman-godara
Copy link
Member Author

Got it! 👍 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement to_lower, to_upper, to_title and reverse for string_type
4 participants