Removed GOTOs

[jacobwilliams]

See #74

Removed all GOTO statements. The logic of the code has not changed, so the results some be identical as before.

[codecov]

Codecov Report

Merging #75 (81ebe7e) into main (dc14d61) will increase coverage by 0.02%.
The diff coverage is 83.24%.

@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
+ Coverage   88.77%   88.80%   +0.02%     
==========================================
  Files           2        2              
  Lines        1221     1224       +3     
  Branches      456      456              
==========================================
+ Hits         1084     1087       +3     
  Misses         40       40              
  Partials       97       97              

Impacted Files	Coverage Δ
src/minpack.f90	88.29% <83.24%> (+0.03%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc14d61...81ebe7e. Read the comment docs.

[ivan-pi]

There seem to be changes in many places besides the few goto's. Presumably due to an indentation change? It makes it hard to review the actual changes.

[jacobwilliams]

Yeah, it's just indenting since I added the new blocks and also removed some unnecessary if blocks. If you look at it with a diff that ignores whitespace it does look a little cleaner.

[certik]

Here is how to get a clean diff:

• https://github.com/fortran-lang/minpack/pull/75/files?w=1

[certik]

I reviewed the simplified diff using the link I posted above, and it looks good, I don't see any bad change.

It'd be nice to get this reviewed by somebody else as well.

I noticed the Codecov is saying some lines are not tested, which is unrelated to this PR, but we should test every line eventually.

[awvwgk]

Overall the change looks good. Is there a better naming for the blocks / loops other than main, outer and inner?

[certik]

@awvwgk thanks for the additional review. Are you ok with merging as is, and then we can improve naming of loop labels in subsequent PRs?

[awvwgk]

Sure go ahead with merging. Best open an issue about naming afterwards.

[certik]

@jacobwilliams go ahead and merge this. If there are any issues, we'll fix it up afterwards.

[jacobwilliams]

we should push something to master to make the test pass... it doesn't look good to have master failing. :)

the failures were parts of the code that were never tested (alternate options)... we can make tests for them... just haven't had time...

[certik]

According to this: https://github.com/fortran-lang/minpack/commits/main, master (main) passes CI. So I don't see a problem?

[awvwgk]

we should push something to master to make the test pass... it doesn't look good to have master failing. :)

Definitely not, if there is a concern about not passing the codecov patch check, it should be addressed in the PR, not by pushing a commit which doesn't affect coverage and automatically passes the patch check.

Personally, I think it is okay fail the codecov check by a small margin, 5% here. The default for the patch check is that everything fails which has a lower patch coverage than the current project coverage. However, I consider the codecov check more like a help for the reviewers, if it flags serious coverage loss or the patch coverage is almost zero, we know there is something wrong.

Of course, it is always preferred to improve the testing to pass the codecov patch check ;). Also, note that a 100% coverage is practically out-of-reach for any Fortran project at the moment due to the way the coverage maps back from the internal representation to the actual source lines.

[jacobwilliams]

weird... the patch test did pass when I merged it... No idea why!! :)