Concise error handling

[ebranlard]

Is your feature request related to a problem? Please describe.

Error handling takes up a lot of space in the code and makes it hard to read.

Describe the solution you'd like

As a new programmer in OpenFAST, I find it difficult at times to follow the code. The first thing I usually do when I enter a piece of code of OpenFAST I've never seen, is that I delete most of the empty lines and the error handling lines. As a rule of thumb, I like when a function does not take more space than a screen in height.

I'm presenting here the approach I'm following for the module ExtPtfm on which I'm currently working. I'd like to trigger a discussion on this issue and see if we can agree on something. I don't suggest changing the existing code, but instead find an approach for new code (or just a green light for the ExtPtfm code I'm writing).

For a given routine, I delete the following lines:

  INTEGER(IntKi)                   :: ErrStat2
  CHARACTER(ErrMsgLen)   :: ErrMsg2 
  CHARACTER(*), PARAMETER    :: RoutineName = 'ExtPtfm_Init'

Instead of these lines, I add the following at the end of the routine:

contains
    logical function Failed()
        CALL SetErrMsg(ErrStat, ErrMsg, 'ExtPtfm_Init')
        Failed =  ErrStat >= AbortErrLev
       if(Failed) call cleanup() ! OPTIONAL, if a cleanup routine exists
    end function Failed

Where the function SetErrMsg is similar to the NWTCIO library function but it does not use the temporary variables ErrStat2 and ErrMsg2.

subroutine SetErrMsg(ErrStat, ErrMess, RoutineName)
  INTEGER(IntKi), INTENT(IN)   :: ErrStat      ! Error status of the operation
  CHARACTER(*),   INTENT(INOUT)  :: ErrMess      ! Error message if ErrStat /= ErrID_None
  CHARACTER(*),   INTENT(IN   )  :: RoutineName  ! Name of the routine error occurred in
  if (ErrStat /= ErrID_None) then
     ErrMess = TRIM(RoutineName)//':'//TRIM(ErrMess)
  end if
end subroutine SetErrStatSimple

Then in the routine, I replace

CALL AllocAry(u%BlPitchCom, p%NumBl, 'BlPitchCom', ErrStat2, ErrMsg2)
CALL SetErrStat(ErrStat2, ErrMsg2, ErrStat, ErrMsg, RoutineName)
IF (ErrStat >= AbortErrLev) THEN
     CALL Cleanup()
     RETURN
END IF

by

call AllocAry(u%BlPitchCom, p%NumBl, 'BlPitchCom', ErrStat, ErrMsg); if(Failed()) return

The function SetErrStat can still be used to set an error message manually.

The solution I'm suggesting only adds the routine SetErrMsg to NWTC_Library and it keeps the error handling philosophy already in place. Yet, it can save thousands of lines and I believe it increases the readability, especially for newcomers. I'm not sure Failed and SetErrMsg are the best names.

Obviously I'm opened to other solutions that would also go in the direction of readability and concise error handling. I just wanted to start the discussion.

Additional option
An additional option, is to use goto. It's a dangerous and controversial construct, but I would argue that it's actually well suited if it's sole use is for error handling at the end of a method. Unfortunately (or fortunately), it's considered obsolete as of Fortran 95.

[bjonkman]

Error handling is often a pain in Fortran, so if you can make it more concise, go for it. However, I have a few comments about what you've proposed:

1. I am very against using goto statements. Don't do it. :)
2. It is tempting to avoid initializing ErrStat/ErrMsg if they get initialized in other routines that are called in the subroutine (e.g., AllocAry). However, I've spent a lot of time over the years debugging issues where the error handling variables are not initialized as a result of someone removing or changing one of those calls later and forgetting to look at the initialization logic at the beginning of the routine. If you do choose to do this, make sure you have a lot of documentation in the routine to avoid introducing these kinds of bugs.
3. I'm not sure how you would get a concatenated error message without using a temporary variable like ErrMsg2. In your logic ErrMsg and ErrStat get overwritten with every call. The original idea was that if you return an error status less than the abort error level (e.g., an informational message, warning, or severe error), the routine would keep going and return all of the messages at once. With your proposed logic, you'd have to print the message immediately or it would be lost. We had decided the glue code should make the decisions about whether those kinds of messages should be printed. (We added a verbosity level in the NWTC_Library, but the logic around that never got implemented. That is also the reason for using the MAX(ErrStat2,ErrStat) ).

[andrew-platt]

I agree with Bonnie on this. It is a pain in Fortran. I like the concept of keeping subroutines and functions small enough that they fit on a screen, but unnecessary. I just collapse the code sections I don't want to see while working on it.

1. Yup. Goto is evil.
2. I find that if I don't do the initializing of error status/messages in a routine, I run into debugging issues later when I decide I can use that same subroutine someplace else. If the subroutine is contained within a larger subroutine, then I get lazy about it. Often though I find I end up migrating those sub-subroutines out later, and then have to go through extra effort to make sure the error handling is good.
3. I very often put lots of extra info level ErrMsgs to hand back to the main code, so losing those would be a hinderance in my opinion.

I would love to see the verbosity level used. I added it to some of the driver codes I wrote (InflowWind driver, 2nd order waves driver codes), along with lots of command line options.

[ebranlard]

Thank you for your feedback! I'm glad to know you also have frustrations on this topic. And to be clear, I'm all up for initializing all variables! (In fact I'm also for initializing all arrays within a do loop. But we can talk about that and gotos in person :) )

In the first solution I suggested above, the concatenation does occur and the caller still gets to decide what to do about the error/warning/info message, but consecutive warnings and info messages within a method would indeed be lost!! This can be avoided by putting the verbose solution into the Failed function as follows:

contains
    logical function Failed()
        CALL SetErrStat(ErrStat2, ErrMsg2, ErrStat, ErrMsg, RoutineName)
        Failed =  ErrStat >= AbortErrLev
        if(Failed) call cleanup() ! OPTIONAL, if a cleanup routine exists
    end function Failed

What I like about this is that the error checking is concise and do not disturb the reading of the main code.

[bjonkman]

Yes, that looks cleaner and now has the same functionality when using SetErrStat and temporary variables.

I'm a little curious about the performance with this additional IF statement. I remember wondering about that when we first implemented SetErrStat (because that also requires one extra IF statement), but never checked it. Hopefully the compiler would be able to optimize it, but if it doesn't, that may be a lot slower. Have you done any profiling on this?

[ebranlard]

I haven't done any profiling neither. It's indeed adding a function call and an if statement. It's probably fine as long as these checks are done for initialization routines that are run only once.
I'll try to keep this in mind for parts of the code that are heavily called.