-
Notifications
You must be signed in to change notification settings - Fork 25
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
simplify strnum code and testing #410
Conversation
src/strnum.F90
Outdated
|
||
! Decode the integer from the string. | ||
|
||
if ( lens .eq. 0 ) return | ||
write ( fmt, '(''(I'',I2,'')'')' ) lens |
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.
Is it necessary to specify the length exactly? If we use a maximum length here, I believe it will work, and we will not need to set the fmt variable.
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'm not sure that will work. Remember the string will be left-justified when it comes back from strsuc, so if we just use the maximum length of 40, then I think we could end up with unexpected results. For example, if any trailing blanks get interpreted as zeroes, then, e.g. "124 "
would come back as 124,000 instead of 124.
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.
So Ed is thinking of something like "read ( str2(1:lens), '(i)' ) num". That should be okay since presumably str2(1:lens) has no trailing blanks, right? I would only worry that some fortran flavors are quirky about internal formatting syntax.
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.
Hmm, OK, I never knew that just (I)
was a valid format specifier - I thought it always needed a number after it, and so I thought what Ed was suggesting was something like read ( str2, '(i40)' ) num
, and which is why I was concerned.
If (I)
is OK (again, I've never seen that one before ;-), then I agree we may be OK with something like read ( str2(1:lens), '(I)' ) num
because there certainly won't be any trailing blanks inside of the str2(1:lens) substring. I'm certainly all for keeping the code as simple as possible, but I just want to make sure we're not sacrificing safety and/or code stability for the sake of a couple of extra lines of code ;-)
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.
Just for sh*ts and giggles, I tried a build using (I)
, but it wouldn't even compile, and which seems to confirm my suspicion:
/lfs/h2/emc/obsproc/noscrub/jeff.ator/NCEPLIBS-bufr-GitHub/nceplibs-bufr/src/strnum.F90:58:25:
58 | read ( str2(1:lens), '(I)', iostat = ios ) num
| 1
Error: Nonnegative width required in format string at (1)
[ 29%] Building Fortran object src/CMakeFiles/bufr_4_f.dir/ufbqcd.f.o
make[2]: *** [src/CMakeFiles/bufr_4_f.dir/build.make:2441: src/CMakeFiles/bufr_4_f.dir/strnum.F90.o] Error 1
So bottom line, I think we've simplified as much as we can, unless I'm missing something here.
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 also tried it by hardcoding the format as (I40)
(the 40 is the hardcoded dimensioned length of str2). That does seem to work, and it also gets rid of the fmt variable, albeit at the cost of a bit of additional hardcoding of the string size.
I can push that up if you think it's a worthwhile tradeoff. Please chime in with your $0.02 - thanks!
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.
Going back now and looking at Ed's original comment, I think this may have been what he was suggesting all along, and Jack and I may have both just misunderstood(?) And since we're only inputting the substring str2(1:lens)
into strsuc (vs. the entire str2
string), then strsuc should never see any of those trailing blanks and it should be OK.
So I'll go ahead and push that up now...
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.
The '(i)' format does work with ifort and some other fortrans, but as mentioned in my previous comment, some fortran compilers probably don't agree. Actually, "read ( str2(1:lens), *, iostat = ios ) num" works in ifort also, but I'm sure not in all compilers either. Anyway hard coding i40 seems to be a more general solution. although it looks funny!
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 meant that you should use a number.
I don't think 40 is right because the maximum signed int is 2,147,483,647, which is only 10 digits.
Absolutely you need a number to make this standard fortran, and standard fortran is what we want.
GNU has a -std flag that I am going to experiment with. It stops allowing GNU extensions and insists on standard fortran everywhere.
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.
OK, we could lower the limit to, say, 12 to allow for a leading negative sign. Either way, whatever we pick should be consistent with how str2 is dimensioned in the routine, and I had picked 40 for that in case the user passes in str1 as a garbage string, or a long string with a lot of leading or trailing blanks, or whatever. Basically, the first thing that happens is that str1 gets an adjustl and len_trim done to it inside of the strsuc subroutine, so str2 needs to be allocated at least as large as the longest string we might ever see in str1.
And yes, I did think about making str2 an allocatable and dynamically allocating it inside of strnum based on len(str1). However, if we do that, then we're back to having a variable (i.e. unknown ahead of time) string length, which means we then need to revert to a write statement to store that length into the fmt string, unless we want to pick something like 12 as a max for the read statement and hardcode that like we're doing now for 40.
Bottom line, and unless I'm missing something, we need to either hardcode some number in the format statement for the read, or else do a preceding write to store the variable length into the fmt string like we had previously. So pick your poison, I guess ;-)
follow-up to #406