doxygen changes

[edwardhartnett]

Part of #246

More doxygen work.

@jbathegit there are some "???" fields to be filled in here...

I believe we should also remove all the "[in]", "[out]", and "[inout]" from bufr_interface.h. These have no meaning for C.

[jbathegit]

I believe we should also remove all the "[in]", "[out]", and "[inout]" from bufr_interface.h. These have no meaning for C.

I strongly disagree. I think it's very helpful for the documentation to be clear about what arguments are input vs. output for any subprogram.

See here for how your proposed changes render bufr_interface.h in HTML. If I were an unfamiliar user trying to determine how to call most of these routines from my C application code, I would be very confused by this without any clear indication of which arguments were input and which were output.

[jbathegit]

I also want to bring @jack-woollen into this conversation for his comments, and he can also help contribute to these documentation updates in the future, as well as serving as a reviewer.

For my part, I'm going to be a bit down in the weeds working on #79 during the coming weeks.

[edwardhartnett]

@jbathegit WRT in/out/inout they do not apply to C code.

in/out/inout only makes sense for F90 code, though here you have applied it to F77 code.

We do not use them on any other C code in NCEPLIBS, and they do not belong here either. Take it from a C programmer, they will not help. Thinking of parameters as in or out is a Fortran thing.

[edwardhartnett]

There is never a convenient time to do documentation, but it is fair and proper that all programmers for NOAA be required to fully document their work.

I know we are all very busy, but putting off documentation tasks does not lead to greater productivity for the organization - quite the opposite. Saving 30 seconds by not writing documentation may cost another programmer days of debugging. Documentation is every bit as important as the code, because without the documentation, the code has far less value to NOAA.

One of the great things about doxygen is that, once the code is fully documented, doxygen will automatically fail any attempt to add code without documentation. So once we can reach a state of documentation completeness, the CI system will ensure that it is maintained.

Every other NCEPLIBS project is now fully doxygenated, and I am confident that we can get NCEPLIBS-bufr there in a few weeks of steady effort.

[jbathegit]

@jbathegit WRT in/out/inout they do not apply to C code.

in/out/inout only makes sense for F90 code, though here you have applied it to F77 code.

We do not use them on any other C code in NCEPLIBS, and they do not belong here either. Take it from a C programmer, they will not help. Thinking of parameters as in or out is a Fortran thing.

I hear what you're saying, but I'm a C programmer myself, and I've never felt that documenting whether something was intended as input or output was just a Fortran thing.

I get that every subprogram in C is technically a function, but sometimes there's more than one output value, so you can't always just assume that every call parameter is input and the return value of the function is the only output. And sometimes the return value has type void, which means that the subprogram is basically a subroutine like in Fortran. So I really don't agree that in/out/inout don't apply when documenting C code. And I think the documentation is really lacking if we don't include this. Again, I'm a C programmer myself, and I really don't understand the reasoning for not doing this.

For example, as a C programmer, if I'm unfamiliar with the inner workings of the library and am trying to figure out from the get_val_f documentation which values need to be provided as input to the function and which I should expect as output, how would I know this? Ditto for the restd documentation. Do you really think this is clear enough to a potential user of the library without explicitly including in/out/inout labels?

[jbathegit]

There is never a convenient time to do documentation, but it is fair and proper that all programmers for NOAA be required to fully document their work.

I know we are all very busy, but putting off documentation tasks does not lead to greater productivity for the organization - quite the opposite. Saving 30 seconds by not writing documentation may cost another programmer days of debugging. Documentation is every bit as important as the code, because without the documentation, the code has far less value to NOAA.

One of the great things about doxygen is that, once the code is fully documented, doxygen will automatically fail any attempt to add code without documentation. So once we can reach a state of documentation completeness, the CI system will ensure that it is maintained.

Every other NCEPLIBS project is now fully doxygenated, and I am confident that we can get NCEPLIBS-bufr there in a few weeks of steady effort.

I totally agree that this needs to be done, but so does issue #79 which is a lot older than #246, and which I thought we had discussed long ago was the next priority after completing #78. So please don't interpret any of this as an argument against doing documentation, but just the priority of it vs. every other open issue. Why is being able to turn on WARN_ON_ERROR suddenly the most pressing issue?

FWIW, some of the restructuring work that I'll be doing in #79 will help improve the documentation, including getting rid of all of the "f77int" typedef occurrences, because we'll now be able to pass arguments by value between the C and Fortran portions of the library, and because we no longer have to worry about 4-byte integers vs. 8-byte integers within different builds. So I was planning to focus on that task and let others focus on #246 and other stuff.

[edwardhartnett]

OK, thanks for the answers for the ??? in this PR.

In terms of the time this effort will take, I don't think it should take much of your time - Alex and I are doing the heavy lifting here.

However, all code must be documented. And it is not right now. Putting off documentation is exactly what got us to this place where we have undocumented code that is part of operational systems, in violation of NOAA rules and common sense. So if other development slows to fill in missing documentation, that is just paying the technical debt that was incurred when undocumented code was committed to the repo.

I would feel more comfortable about new work happening if we could be sure that all new work is also fully documented. I don't want to be filling in documentation while more undocumented code continues to be added. We need to close the loop here, so there is no longer any benefit to individual programmers to skip documentation. Once we have WARN_AS_ERROR turned on in the CI run, then all code must be documented before it can be committed.

In terms of in/out for C programming, C does not have in/out, it has pass by value/pass by reference. This is well-understood by C programmers, and no other C project feels the need to use Fortran in/out in their docs, so let's not do it here. Let us document our C like all other NCEPLIBS C code, which does not try and assign in/out/inout as if the parameters were fortran.

If you have a pointer parameters, you say what happens in the doxygen. For example: https://docs.unidata.ucar.edu/netcdf-c/current/group__dimensions.html#ga6a1e2683ca1acf91437b790979ac7b8a

The way I usually phrase it for a pointer is like this, for a pointer p that is going to get some value in the function:

@param p Pointer which gets the number of whatevers.

If it's a pointer to a pointer I would say something like:

@param p Pointer which gets the pointer to struct whatever.

We already know how to document C code in Doxygen - it is a well-developed art at this point. No need to attempt to originate new ways to do this - let's just do what everyone else does.

There are a bunch of C prototypes which were undocumented in bufrlib.h. Can you tell me what these functions are?

[edwardhartnett]

@jbathegit what are all these C prototype functions?

[jbathegit]

These are prototypes to the library subprograms of the same name, some of which are C subprograms, but most of which are Fortran subprograms called from C. The latter will go away once #79 is completed, b/c those will instead be handled via new BIND(C, ) attributes. And all of the f77 typedefs will go away as well at that point. But for now, since we're still using the old UNDERSCORE append way of linking C to Fortran, then that's why we have to list all of these here, along with supplemental size_t arguments for prior character string arguments.

For documentations purposes, is it enough to just say it's a prototype to the subprogram of the same name, since the subprogram documentation explains everything in further detail? And for the variable names, could we just copy the same names and same definitions that are listed in the subprogram documentation? Again, most of these prototypes will go away once I get a chance to work on #79.

[jbathegit]

How about I just go ahead and approve and merge this PR as is, and I promise I'll fill in all of the bufrlib.h.in prototype information myself as I work on #79? Again, there will be far fewer prototypes to deal with once that's done.

That way, it will free you and Alex up to move forward on documenting other areas of the library. I'll agree to follow the formatting and examples you've laid out in this PR and get the rest of the C code up to snuff as I work on #79, if you guys can focus your efforts on doxygen updates for all of the remaining Fortran subprograms. I'll remain available to answer questions if you have them, but this way (i.e. if you guys focus on the Fortran parts and I focus on the C parts), then we lessen the chances of stepping on each others' toes as we work in parallel. All of the Fortran subprograms contain documentation, but the documentation just needs to be converted to doxygen-style, as you've seen.

Sound good?

[jbathegit]

Respectfully, and responding to your original reply, it's never a waste of my time to ask questions when I really don't understand something. If you know anything about me, you know that I'm never shy about speaking up when I disagree with something. And I still believe the C documentation is lacking without clear in/out/inout labels. However, I appear to be in the minority here, so I will drop this point now and accept the will of the majority.

[jbathegit]

That said, and as I've mentioned often in the past, I really do appreciate the help from you, Alex, and others to get this library where it needs to be, so please don't ever interpret any of my questions as anything other than an attempt at honest debate.

Also, please remember that Jack Woollen is also a resource who can help answer some of these ??? questions as well, in case I'm busy with other tasks. Going forward it would be best to tag both of us as reviewers on these PRs, and that way either one of us can chime in to help when needed.