-
Notifications
You must be signed in to change notification settings - Fork 134
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
Extend string
interface in fms_string_utils_mod
#1142
Conversation
Replace "sucess" with "success" and "sucessful" with "successful".
Replace "they" with "the".
Use the name "optional_flag" instead of "optional", because the latter is a Fortran keyword.
Extend and improve `string()` in `fms_string_utils_mod`. `r4_kind` and `r8_kind` versions of `string_from_real` have been implemented. Leading and trailing whitespace are now stripped from the returned string. Functions to convert 1D, 2D, and 3D arrays to strings have been implemented and added to the `string` interface.
A unit test for `string()` from `fms_string_utils_mod` has been implemented.
In relation to the extension of the `string` interface in `fms_string_utils_mod`, the new include directory has been added to `CMakeLists.txt` and the new include files have been added to `string_utils/Makefile.am`.
`ifort` doesn't like spaces between real constants and kind values, so the spaces have been removed.
Use explicit format strings in `string_from_r4()` and `string_from_r8()`. This makes the string conversions consistent across compilers, allowing the unit test to pass on both the GNU and Intel compilers. This change required `string_from_r4` and `string_from_r8` to be moved back from the include file into `fms_string_utils.F90`, since the two functions now use different format strings.
Add a function for Boolean values to the `string` interface. Add three new test cases to the unit test.
Create a `stringify` interface in `fms_string_utils_mod`. Move the real array stringification functions from `string` to `stringify`.
Accept an optional format string as the second argument to `string_from_r4()`, `string_from_r8()`, and `stringify()`. If no format string is supplied, `*` is used.
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 need to think about this some more.
Increase the length of the temporary string in `string_from_r4`/`string_from_r8` from 25 to 32 characters.
|
||
class default | ||
call mpp_error(FATAL, "string() called with incompatible argument") | ||
end select | ||
end function | ||
|
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.
please put the name of the function here
end function string
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.
Addressed in 16356ac.
function string(v, fmt) | ||
class(*), intent(in) :: v ! Value to be converted to a string | ||
character(*), intent(in), optional :: fmt !< Optional format string for a real argument | ||
character(:), allocatable :: string |
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.
If it's always being allocated to a length of 32, why not just make this a len=32 string instead of allocatable? I'm not saying you have to do this, I'm just wondering if you think that's a better option. That would also make this closer to being a pure/elemental function.
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.
By making it allocatable
, leading and trailing whitespace can be stripped so that the user doesn't need to do trim(string(x))
. For me at least, this outweighs the benefit of making the function elemental
(although I'm biased by the fact that I've only used string()
for generating unit test log messages).
string_utils/fms_string_utils.F90
Outdated
string = v | ||
|
||
class default | ||
call mpp_error(FATAL, "string() called with incompatible argument") |
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.
if this check was not writing to stdout, then this might be able to be an elemental function
string_utils/fms_string_utils.F90
Outdated
string = "False" | ||
endif | ||
|
||
type is (integer) |
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.
can you add integer(i4_kind)
and integer(i8_kind)
?
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.
Addressed in 16356ac.
|
||
type is (real(r4_kind)) | ||
allocate(character(32) :: string) | ||
if (present(fmt)) then |
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.
idk if you can check this, but how will you know that the user's format will work?
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 think fmt
validation might not be worth the effort, since Fortran will just throw a runtime error if the format string is invalid. The most catastrophic scenario I can imagine would be a format string wider than 32 characters, but the runtime library seems to check this and terminate the program if this happens. It might be a good idea to add a unit test for this case.
string_utils/fms_string_utils.F90
Outdated
|
||
type is (integer) | ||
allocate(character(32) :: string) | ||
write(string, '(i0)') v |
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.
If the user provides a format statement for an integer or a logical or a string, it will be unused. This could produce unexpected behavior for the user.
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.
Addressed in 16356ac via a warning message (in the case of a logical value) and by using fmt
(in the case of an integral value).
string_utils/fms_string_utils.F90
Outdated
type is (character(*)) | ||
string = v |
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.
You should avoid this. The class(*) string handling is inconsistent across compilers and this will only lead to problems.
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.
Removed in 16356ac.
string_utils/fms_string_utils.F90
Outdated
string = v | ||
|
||
class default | ||
call mpp_error(FATAL, "string() called with incompatible argument") |
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.
If you keep the error message, it should be more comprehensive. "string() called with incompatible argument. Must be of type integer(kind=4), integer(kind=8), real(kind=4), real(kind=8), or logical."
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.
Addressed in 16356ac.
string_utils/fms_string_utils.F90
Outdated
!> @brief Converts a number or a Boolean value to a string | ||
!> @return The argument as a string | ||
function string(v, fmt) | ||
class(*), intent(in) :: v ! Value to be converted to a string |
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 variable is not correctly doxygenized
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.
Addressed in 16356ac.
class(*), intent(in) :: v ! Value to be converted to a string | ||
character(*), intent(in), optional :: fmt !< Optional format string for a real argument | ||
class(*), intent(in) :: v !< Value to be converted to a string | ||
character(*), intent(in), optional :: fmt !< Optional format string for a real or integral argument |
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.
There is a typo. Should be "integer" not "integral"
* fix - gnu and pgi issue with class(*) in send_data3d (NOAA-GFDL#1149) * feat: extend `string` interface in `fms_string_utils_mod` (NOAA-GFDL#1142) * fix: add omp directives for race condition in tridiagonal (NOAA-GFDL#1109) * fix: missing if statement in PR NOAA-GFDL#1149 (NOAA-GFDL#1155) * fix: time_manager missing changes from year to yr, month to mo, day to dy (NOAA-GFDL#1169) * chore: update changelog and version numbers for release (NOAA-GFDL#1167) * chore: append dev to version number post-release (NOAA-GFDL#1179) --------- Co-authored-by: Niki Zadeh <niki.zadeh@noaa.gov> Co-authored-by: Jesse Lentz <42011922+J-Lentz@users.noreply.github.com> Co-authored-by: ganganoaa <121043264+ganganoaa@users.noreply.github.com> Co-authored-by: MiKyung Lee <58964324+mlee03@users.noreply.github.com> Co-authored-by: Ryan Mulhall <35538242+rem1776@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Description
The
string
interface infms_string_utils_mod
has been extended to support the following types of input:r4_kind
andr8_kind
realsAdditionally, leading and trailing whitespace is now stripped from the output for real arguments.
Fixes #1141
How Has This Been Tested?
A unit test has been implemented. The test passes on the AMD box with
gcc 12.2.0
.Checklist:
make distcheck
passes