Skip to content

Conversation

arjenmarkus
Copy link
Member

The comments to the previous pull request are now all solved:

  • Extra parentheses have been removed (for readability those around the logical disjunctions and conjunctions have been kept)

  • Some clean-up of the comments (more explicitly stating the ranges)

  • The functions to_lower and to_upper have been made robust (perhaps slower than before, but the performance is certainly not bad)

Suggestion for further improvement: allow to_lower and to_upper to convert strings of arbitrary length - this seems to me to be the most common usage.

arjenmarkus and others added 5 commits September 16, 2020 17:39
Correct the is_lower function - I overlooked that one.
Solve a glitch in the function is_printable
Co-authored-by: Ian Giestas Pauli <iangiestaspauli@gmail.com>
Co-authored-by: Ian Giestas Pauli <iangiestaspauli@gmail.com>
… functions

This commit  cleans up the code a bit (the extra parentheses in several functions, leftover semicolons) and also implements a robust version of the to_lower and to_upper functions.
@jvdp1 jvdp1 mentioned this pull request Sep 24, 2020
@ivan-pi
Copy link
Member

ivan-pi commented Sep 25, 2020

Suggestion for further improvement: allow to_lower and to_upper to convert strings of arbitrary length - this seems to me to be the most common usage.

This has been requested as part of the strings module. I have wondered already if it would be easier to just have a single function with the interface:

pure function upper(str) result(ustr)
character(len=*), intent(in) :: str
character(len=len(str)) :: ustr
end function

and get rid of the single character case conversions entirely. As you say, the most common usage will be arbitrary length strings, so I am not sure if we need a specialization for single letters. The arbitrary length version can be used on single characters too (hopefully the compilers are capable enough to remove the loop for characters with len=1).

The reason I included the case conversions in the first plase was just to cover the same functionality as the <ctype.h> header file from C.

@milancurcic
Copy link
Member

@arjenmarkus @ivan-pi @14NGiestas Is this PR good to go or should this comment still be addressed?

@arjenmarkus
Copy link
Member Author

Argh, I had intended to rely to these remarks, but never got around to it. I agree: in C the situation is completely different - there are characters and there are arrays of characters. In Fortran we have character strings but not a separate character type. The suggested interface (and almost trivial implementation) looks good to me.

@milancurcic
Copy link
Member

Okay, I will merge this then. The suggested interface can go in as a new PR.

@milancurcic milancurcic merged commit d0cc617 into master Nov 13, 2020
@milancurcic milancurcic deleted the update_ascii branch November 13, 2020 15:07
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.

4 participants