Use modern fortran/C API about nceplibs-bufr HOT 13 CLOSED

Now that I see the state of the documentation, I believe this task should go on the back burner until all current code is fully and correctly documented.

What is the point of adding more undocumented code to the system? We are just digging the hole deeper.

from nceplibs-bufr.

Fair point, but you're assuming that any code I add is undocumented. I take great pains to make sure I document everything I contribute. The issue is with all of the legacy code, much of which is in fact documented, but just not the way doxygen likes it.

from nceplibs-bufr.

There are ~100 problem files remaining, and I am doing 10 per day, in about 30 minutes of effort. If you, @jbathegit would do the same, and you also @jack-woollen, then this task would be complete by the end of next week.

Let's get the documentation in order before moving on to any other code changes, unless there is an urgent operational need or bug that has to be fixed. If you are impatient to get on to other work, I invite you to dig into this documentation task and get it done sooner.

from nceplibs-bufr.

@edwardhartnett, we've already discussed this elsewhere, and I'm fine with focusing on the documentation first if you really feel that's more important. The reasons I had initially been planning to do this one next is b/c it's an even older issue and we'd finally gotten #78 completed. Plus, once this C-Fortran interface was modernized, a lot of the existing prototypes in bufrlib.h.in and elsewhere would completely go away, so less to document and better the chances that doxygen would do it correctly.

But again, I'm willing to focus on the documentation first, and that's what I've been doing for the past few days. In fact, I've spent at least 6 hours this week on this task, contributing my own documentation updates and edits plus reviewing multiple updates from you and Alex and then getting them all approved and merged in an orderly fashion, and sometimes fighting with the GitHub runner in the process (as you've seen). So please don't post a snarky comment that seems to suggest I haven't put in at least 30 minutes per day towards this task.

As I've also mentioned several times, I sincerely appreciate any and all help from you, Alex, and others to get this done (and you too Jack! ;-). But I also believe I've already been pulling my own weight and then some towards this goal, so for you to post a comment which suggests otherwise is a bit offensive.

from nceplibs-bufr.

I would suggest that before any further code changes are made we should have #308, #300, and #244 finished. Also that we ensure we have full testing for all C functions with the existing code...

from nceplibs-bufr.

Testing for the C code is already included to the same extent as for the Fortran code. In other words, not perfect, but over 70% coverage throughout the entire library.

I'm willing to help with these other tasks, but I just want to point out (again, and for the record) that this issue needs to be resolved before we can do the next v12.0.0 release. So continuing to put it off isn't going to help us in that regard.

from nceplibs-bufr.

Does this issue need to be resolved before the next release?

And is there a reason for an release soon? THat is, is someone waiting for a release for a bugfix or important new feature?

If you are exposing a C API, we need tests for every function you expose, in C. How can we be sure the C api is working as intended without C tests for every function? Furthermore, these tests are needed for debugging and also to demonstrate how to use the C API.

In addition, here are some C codes that really need more testing. Look at the code coverage report to the untested code:

• crbmg.c
• cwbmg.c
• dlloctbf.c

from nceplibs-bufr.

No, nobody is beating down our door at the moment for a new release. But as I've noted previously, the next release will be a major (e.g. v12.0.0) release because of all of the work Jack and I previously did to consolidate down to one build, and which now requires any _8 application codes to include a call to the new subroutine setimb8. And since resolving this particular issue will also require any Fortran application codes which call any of the C library routines to now also include a new "use" call to a new module, then it makes sense to do both of these as part of the same major release. Otherwise, the next follow-on release would have to be yet another major (e.g. 13.0.0) release.

BTW, crbmg.c is already being tested in intest1 and test_IN_4. But that said, yes I'm aware that there are a few remaining C codes that still need tests.

from nceplibs-bufr.

OK, good to know.

from nceplibs-bufr.

Another place this UNDERSCORE is still in use is cread.c, and that's making it challenging to test.

from nceplibs-bufr.

Yeah, it's still in a lot of places. I resumed working on this issue last week with #412, and I'll eventually get to all of them.

from nceplibs-bufr.

OK, this is not quite correct:

!> @fn bufr_interface::cobfl_c::cobfl_c(bfl, io)   
!> Wraps BUFRLIB cobfl() function.
!>
!> @param bfl - [path]/name of system file to be opened
!> @param io - Flag indicating how bfl is to be opened
!>
!> @author J. Ator @date 2023-03-22
subroutine cobfl_c( bfl, io ) bind(C, name='cobfl')
  use iso_c_binding
  character(len=1), intent(in) :: bfl(*)
  character(len=1), intent(in), value :: io
end subroutine cobfl_c

We need the types to be C types. See these examples from the NetCDF Fortran API:

 Function nc_inq_ncid(ncid, name, grp_ncid) BIND(C)

 USE ISO_C_BINDING, ONLY: C_INT, C_CHAR

 Integer(C_INT),         VALUE         :: ncid
 Character(KIND=C_CHAR), Intent(IN)    :: name(*)
 Integer(C_INT),         Intent(INOUT) :: grp_ncid

 Integer(C_INT)                        :: nc_inq_ncid

 End Function nc_inq_ncid
End Interface
 Function nc_insert_array_compound_f(ncid, xtype, name, offset, field_typeid, &
                                     ndims, dim_sizes) BIND(C)

 USE ISO_C_BINDING, ONLY: C_INT, C_SIZE_T, C_CHAR

 Integer(C_INT),         VALUE         :: ncid, ndims
 Integer(C_INT),         VALUE         :: xtype, field_typeid  ! nc_type in C
 Integer(C_SIZE_T),      VALUE         :: offset
 Character(KIND=C_CHAR), Intent(IN)    :: name(*)
 Integer(C_INT),         Intent(INOUT) :: dim_sizes(*)

 Integer(C_INT)                        :: nc_insert_array_compound_f

 End Function nc_insert_array_compound_f

from nceplibs-bufr.

Recently in #480 the point was made that changing to the modern C/Fortran is causing the API to change.

@jbathegit I don't understand why the C function signatures have to change...

from nceplibs-bufr.