update strnum to handle valx functionality, and remove valx #406
Conversation
| ! Decode the integer from the string. | ||
|
|
||
| write ( fmt, '(''(I'',I2,'')'')' ) lens | ||
| read ( str2(1:lens), fmt ) num |
There was a problem hiding this comment.
If non-numeric characters are in the string, won't this fail? In which case, why try to detect non-numeric characters? Why not just try to read and if there is a failure, report a failure?
There was a problem hiding this comment.
IMHO, the prior verify check was a cleaner and clearer way to confirm that str2(1:lens) contained only valid characters, and in such cases to go ahead and report the failure by setting iret to a value of -1.
That said, I certainly could change this to add an err= option to the read statement and have it reroute control to a new numbered statement to set iret = -1 and return that way, if you and/or Jack think that would be a better way to do it. Again, this was just my personal preference.
There was a problem hiding this comment.
OK, I just saw your #409 (sorry, it's hard keeping track of all of these overlapping conversations and PR's flying around ;-)
So I take it you would indeed prefer that I just modify the new strnum.F90 to add an iostat= check to the read and remove the verify check, and I see where you've already included some very thorough strnum testing in test_misc.F90, so I'll go ahead and remove my less thorough tests from intest7.F90 as well.
There was a problem hiding this comment.
We want to reduce code to the smallest amount that can meet current requirements. This is the lowest cost for NOAA over the long term, and the least amount of work for us.
So if using iostat can save us a few lines of code, those are lines NOAA does not have to pay for. Recall that maintenance costs for a line of code are an order of magnitude more than the cost to write the line of code. Code is not free to keep, it's like owning a horse - it costs a lot of money even if you don't use it much.
We must always prefer the solution that meets current requirements with the least lines of code.
|
|
||
| ! Verify that the string contains all legal characters. | ||
|
|
||
| call strsuc ( str, str2, lens ) |
There was a problem hiding this comment.
Why strsuc? Can't trim() do this already?
There was a problem hiding this comment.
If you look at my new strsuc in this PR (where I've completely rewritten it and migrated it to F90), you'll see that it now does nothing more than call the intrinsic functions adjustl and len_trim ;-)
It's true that, after having done that, I could have just done away with strsuc altogether and just called adjustl and len_trim directly from within strnum.F90. But then I'd have to do likewise in a bunch of different subprograms in the library which also call strsuc, so I decided (again, just IMHO) to do it this way.
|
This is a solid improvement. I suggest you merge this and we continue discussions in #369. Removing valx() is a great thing. |
|
OK, I'll do that, and I see you've now closed #409. But I'll still follow up with another PR of my own which follows your advice in #409, and which cleans up strnum.F90 even further and moves the testing for some of those oddball cases from intest7.F90 to test_misc.F90, like you demonstrated. I'll do that tomorrow morning... |
Fixes #369
Fixes #407
Part of #254
Part of #300