search

NOAA-EMC / NCEPLIBS-bufr

The NCEPLIBS-bufr library contains routines and utilites for working with the WMO BUFR format.
Other
44 stars 19 forks source link

goto considered harmful? #329

Closed edwardhartnett closed 1 year ago

edwardhartnett commented 1 year ago

Legendary programmer Dijkstra made the case against goto in his short and famous paper: Go To Statement Considered Harmful (https://homepages.cwi.nl/~storm/teaching/reader/Dijkstra68.pdf)

Recently the folk at NCO have apparently agreed with Dijkstra and asked @GeorgeGayno-NOAA to remove gotos from some UFS_UTILS code,

A valued co-programmer of netcdf-c, @DennisHeimbigner , makes a strong case in favor of forward-only gotos, used only to do resource cleanup and then exit functions. And this seems to be how GOTO is used in NCEPLIBS-bufr. Will NCO agree with this distinction?

Others will say that function cleanup can be accomplished without gotos, simply by using appropriate error handling in the function, and throughout the library, and then checking previous error codes before continuing. This may however use more lines of code than GOTO.

In this issue I wanted to open the discussion of GOTOs and whether they should continue to be used in our Fortran codes.

Does anyone know whether other teams are doing anything to remove GOTOs?

edwardhartnett commented 1 year ago

@aerorahul @AlexanderRichert-NOAA @Hang-Lei-NOAA @GeorgeVandenberghe-NOAA @GeorgeGayno-NOAA your thoughts would be welcomed on this issue for NCEPLIBS-bufr and other NCEPLIBS libraries.

GeorgeVandenberghe-NOAA commented 1 year ago

Removing GOTOs from a code that has them should be treated for cost and testing reasons as a refactoring. I have since first programming course, followed a modified style rule of always branching down to one further DOWN in a code, never up but for a working code that has them, best just leave it alone until it is heavily refactored for other reasons which would require thorough retesting of the whole code anyway.

On Sat, Feb 18, 2023 at 5:23 AM Edward Hartnett @.***> wrote:

@aerorahul https://github.com/aerorahul @AlexanderRichert-NOAA https://github.com/AlexanderRichert-NOAA @Hang-Lei-NOAA https://github.com/Hang-Lei-NOAA @GeorgeVandenberghe-NOAA https://github.com/GeorgeVandenberghe-NOAA @GeorgeGayno-NOAA https://github.com/GeorgeGayno-NOAA your thoughts would be welcomed on this issue for NCEPLIBS-bufr and other NCEPLIBS libraries.

— Reply to this email directly, view it on GitHub https://github.com/NOAA-EMC/NCEPLIBS-bufr/issues/329#issuecomment-1435624232, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANDS4FRFCZRGB4YHP5IYUU3WYCPK3ANCNFSM6AAAAAAVAHUCKU . You are receiving this because you were mentioned.Message ID: @.***>

--

George W Vandenberghe

Lynker Technologies at NOAA/NWS/NCEP/EMC

5830 University Research Ct., Rm. 2141

College Park, MD 20740

@.***

301-683-3769(work) 3017751547(cell)

DennisHeimbigner commented 1 year ago

My intent in using (forward) goto in the netcdf C library is as Ed notes. In effect, I am simulating the "Try { ... } Finally" construct of Java. The underlying reason is to ensure that all malloc'd memory is reclaimed at a single location at the end of the function. This approach has been remarkably good at preventing memory leaks. With one exception, refactorings to remove goto result in having to duplicate memory reclaiming at multiple points in the function. This inherently introduces consistency problems that lead to memory leaks. The one alternate way to simulate the equivalent of using goto in C is by using nested"do {...} while(0)" loops with labels and break statements. This approach is very opaque and it is difficult to argue in its favor.

jbathegit commented 1 year ago

My $0.02 is that goto itself is not an inherently bad thing, but that it's gotten a very bad rap over many years because too many programmers have insisted on abusing it, in lieu of taking the time to design and write better-structured code. If you think about it, what a goto does isn't really much different than using a break or continue statement, or having multiple possible return statements within a subprogram. But again, it can also be easily abused by lazy programmers to just jump to any random numbered line in a code, and which is why I think a lot of people now completely hate it.

So bottom line, I think for NCO or anyone to just make a blanket admonition of "No more goto statements in any code" is way too harsh and can cause a lot of very otherwise good or clever programming to have to be refactored for no good reason. Instead, goto occurrences really need to be looked at on a case-by-case basis to analyze whether they're really violating the letter and spirit of structured programming. At least in NCEPLIBS-bufr, I don't think there are any such occurrences.

GeorgeVandenberghe-NOAA commented 1 year ago

I agree completely with Jeff Ator on this and started encountering overreaction to GOTO presence back in the late 70s when I first started PL/1 programming (GOTO was essential for fortran 66 of that time)

And refactoring a working , well encapsulated, code we're otherwise not going to modify is expensive and risky.

On Fri, Mar 10, 2023 at 5:45 PM Jeff Ator @.***> wrote:

My $0.02 is that goto itself is not an inherently bad thing, but that it's gotten a very bad rap over many years because too many programmers have insisted on abusing it, in lieu of taking the time to design and write better-structured code. If you think about it, what a goto does isn't really much different than using a break or continue statement, or having multiple possible return statements within a subprogram. But again, it can also be easily abused by lazy programmers to just jump to any random numbered line in a code, and which is why I think a lot of people now completely hate it.

So bottom line, I think for NCO or anyone to just make a blanket admonition of "No more goto statements in any code" is way too harsh and can cause a lot of very otherwise good or clever programming to have to be refactored for no good reason. Instead, goto occurrences really need to be looked at on a case-by-case basis to analyze whether they're really violating the letter and spirit of structured programming. At least in NCEPLIBS-bufr, I don't think there are any such occurrences.

— Reply to this email directly, view it on GitHub https://github.com/NOAA-EMC/NCEPLIBS-bufr/issues/329#issuecomment-1464153039, or unsubscribe https://github.com/notifications/unsubscribe-auth/ANDS4FWECHGX4Y5QQ7TJOJTW3NSFBANCNFSM6AAAAAAVAHUCKU . You are receiving this because you were mentioned.Message ID: @.***>

--

George W Vandenberghe

Lynker Technologies at NOAA/NWS/NCEP/EMC

5830 University Research Ct., Rm. 2141

College Park, MD 20740

@.***

301-683-3769(work) 3017751547(cell)

edwardhartnett commented 1 year ago

We have all agreed that:

edwardhartnett commented 1 year ago

I would like to add that when resources don't have to be released, then a forward goto does not have to be used.

I see this pattern in some subroutines:

if(some_error) goto 100

100 return

That's not needed. Just return when you can do so. A forward goto is only needed if there are some resouces that have to be released before returning, and there are multiple places where you want to return in case of errors.