Quiet compiler warnings for registry.exe

[DWesl]

Quiet the compiler warnings in registry.exe

TYPE: bug fix

KEYWORDS: compiler warnings, snprintf, initial values, string length

SOURCE: "Daniel Wesloh (Pennsylvania State University Department of Meteorology and Atmospheric Science)"

DESCRIPTION OF CHANGES: Problem: Generally or specifically, what was wrong and needed to be addressed? A few years ago, registry.exe was segfaulting while compiling WRF. I enabled every compiler warning for that build process I could find, and tried to fix every one I saw. The problem ended up being insufficient stack space (fix merged a few years ago), but fixing the warnings probably won't hurt.

Solution: What was down algorithmically and in the source code to address the problem?

• make sure string buffers are large enough for the contents put in them
• don't use larger types than necessary (i.e. use short instead of int for numbers in the 300--16,000 range, so printf knows that five characters will be enough)
• Specify maximum lengths to copy using snprintf and friends
• Make sure all variables are initialized.

ISSUE: None

LIST OF MODIFIED FILES: list of changed files <!--(use git diff --name-status master to get formatted list)-->

M       configure
M       doc/README.cygwin.md
M       inc/streams.h
M       tools/Makefile
M       tools/data.h
M       tools/fseek_test.c
M       tools/gen_allocs.c
M       tools/gen_args.c
M       tools/gen_config.c
M       tools/gen_defs.c
M       tools/gen_interp.c
M       tools/gen_irr_diag.c
M       tools/gen_scalar_indices.c
M       tools/gen_streams.c
M       tools/gen_wrf_io.c
M       tools/misc.c
M       tools/protos.h
M       tools/reg_parse.c
M       tools/registry.c
M       tools/registry.h
M       tools/set_dim_strs.c
M       tools/sym.c
M       tools/sym.h
M       tools/symtab_gen.c
M       tools/type.c

TESTS CONDUCTED:

1. Do mods fix problem? How can that be demonstrated, and was that test conducted?
   • DWesl/WRF#3
   • WRF compile finishes with this patch
2. Are the Jenkins tests all passing?
   • I have no idea how to run those

RELEASE NOTE: Eliminate some compiler warnings when building registry.exe

[weiwangncar]

@DWesl It looks like the compilation is failed for all of the tests. Our regression tests are run using gfortran compiler. Here is a particular error (also see the attached file):

make[2]: Entering directory '/wrf/WRF/tools' gcc -DIWORDSIZE=4 -DMAX_HISTORY=25 -DNMM_CORE=0 -c -g registry.c registry.c:7:11: fatal error: io.h: No such file or directory 7 | # include <io.h> | ^~~~~~

output_3.txt

[DWesl]

Interesting. My own tests all used GFortran, so it's likely a Cygwin/Linux difference rather than a compiler one. I set up guards so the #include <io.h> should only get seen on Windows and Cygwin and not on Linux, so hopefully that problem goes away.

[weiwangncar]

OK, now our regression tests have passed:

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None

[DWesl]

OK, now our regression tests have passed:

As have mine on Cygwin (with #1812)

[DWesl]

Am I missing anything for this PR?

[weiwangncar]

@islas Are any of the changes proposed here in conflict with anything you've been working on?

[islas]

@weiwangncar Nope, no conflicts should arise from our respective changes in registry

[weiwangncar]

@islas Great! Should we consider this in develop or for 4.5.2?

[islas]

I'd say this would be a nice-to-have if we can get it in, but not critical.

[weiwangncar]

@islas Thanks. We can leave it on the develop branch.

[weiwangncar]

@DWesl The compilation failed in our regression test. If you need to see the output, let me know.

[weiwangncar]

The regression test results from the last commit:

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None

[weiwangncar]

@islas Is this ready for approval?

[islas]

@weiwangncar I think @DWesl has a lot of self-marked revisions they are still going through. Is that correct?

[DWesl]

I completed all the simple work I noticed on a second passthrough. There's a few places where I would like feedback on style.

[DWesl]

Something that should be noted before the final merge: I accidently added a Cygwin workflow in GitHub actions; I'm not sure if the maintainers want to keep that in or out of the main repository.

EDIT: I added the entire "Fix Cygwin compile" PR to this one. Should I rebase the branch to drop those changes? Or just edit them out and let the merge sort out differences?

[weiwangncar]

@DWesl Thanks for removing the Cygwin changes! @islas Are we ok with this PR?

[islas]

I still see some Cygwin changes to configure and the workflow. I'm unsure if that is something we will want to add, though having a test would be incredibly helpful. That content & discussion might be more suited to a Cygwin-specific PR

[DWesl]

I split off the remaining Cygwin-specific fixes to DWesl/WRF#5. This should be just the bits that address warnings, with comments on the bits I thought might be controversial.

[weiwangncar]

@DWesl This PR has code conflicts with what is in the latest repository. Can you resolve these conflicts while preserving the latest change in the repository?

[DWesl]

There's a number of places that change !(p->node_kind & BIT_MASK) to (!p->node_kind) & BIT_MASK, which is basically always going to return false. Could you point me to the PR that made that change and the reasoning for it? My inclination otherwise would be to revert that change or to make it (~p->node_kind & BIT_MASK).

[DWesl]

Apparently that's #1942, which also attempted to fix warnings, in this case a warning that ! p->node_kind & BIT_MASK wasn't doing what was most likely wanted (extracting the bits corresponding to the mask then inverting the result), but was instead applying a logical-not to a bit sequence (which results in either 1 or 0) and then applying the bit mask (here 8, so the result would always be zero). Should I go through and change those to be either a bitwise-invert or apply the parentheses implied by the warning (so the bit-mask gets applied before the logical-not)?

The other option would be to just delete the conditional, but the comment implies there's a point to it. I just asked on that PR for clarification of what I should do with those cases.

[DWesl]

Discussion resolved there, conflicts resolved here: I kept the bit-mask then logical-not I already had.

[DWesl]

Should I try to squash the changes into fewer commits?

[weiwangncar]

@islas Can you see if this PR can be accepted?