Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

more bort tests, some documentation fixes, some misc tests #415

Closed
wants to merge 23 commits into from

Conversation

edwardhartnett
Copy link
Contributor

@edwardhartnett edwardhartnett commented Mar 25, 2023

Part of #381
Part of #397

Testing more borts().

@edwardhartnett edwardhartnett changed the title more bort tests more bort tests, some documentation fixes, some misc tests Mar 26, 2023
C> @param[out] IRET - integer: return code:
C> - 0 MESG was successfully read.
C> - 11 MESG contained a DX BUFR table message.
C> - -1 MESG contained an unrecognized Table A message type.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again for the record, IMO the removal of all of this whitespace makes the docblock a lot less readable. Not everyone relies on just the rendered html from Doxygen for documentation - some of us still occasionaly edit the actual library routines and read it there.

C> This subroutine returns the bit-wise representation of the FXY value corresponding to, sequentially,
C> a particular (IENT'th) "child" mnemonic of a Table D sequence ("parent") mnemonic.
C> Returns the integer FXY value corresponding to a child
C> mnemonic of a Table D sequence parent mnemonic.
Copy link
Collaborator

@jbathegit jbathegit Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've always taken care to explicitly call this the "bit-wise representation of the FXY value", rather than just an "integer FXY value".

For example, if the character representation of the descriptor is "063022", then one could wrongly assume that the corresponding "integer FXY value" is just 63022, but instead what we're really talking about here is a value of 16150. See the remarks in the docblock for ifxy() for a detailed example.

Bottom line, there's a reason we've always referred to the latter value as the "bit-wise representation of the FXY value" throughout the entire library, rather than just the "integer FXY value" :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, the problem is that "bit-wise representation of the FXY value" is quite confusing. Even with your explanation, I don't understand the different between 63022 and 16150. Let's discuss at the meeting today...

@jbathegit
Copy link
Collaborator

jbathegit commented Mar 27, 2023

BTW, I noticed in the developer CI run that a number of your new ufbstp tests were failing due to a SIGSEGV, rather than failing within the bort routine like we want:

Program received signal SIGSEGV: Segmentation fault - invalid memory reference.

Also, at least two new tests are failing with memory leaks:

1: ==6968==ERROR: LeakSanitizer: detected memory leaks
1: 
1: Direct leak of 32 byte(s) in 1 object(s) allocated from:
1:     #0 0x7fb0dc424867 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
1:     #1 0x7fb0dc0b7ef8  (/lib/x86_64-linux-gnu/libgfortran.so.5+0x22ef8)
1: 
1: Indirect leak of 512 byte(s) in 1 object(s) allocated from:
1:     #0 0x7fb0dc424867 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
1:     #1 0x7fb0dc0b7ef8  (/lib/x86_64-linux-gnu/libgfortran.so.5+0x22ef8)
1: 
1: SUMMARY: AddressSanitizer: 544 byte(s) leaked in 2 allocation(s).

Of course it can be really hard to notice these things with test_bort, since the roles are reversed and a fail now gets interpreted as a pass. So I think we really need to get in the habit of digging through the developer run output to check for these.

@edwardhartnett
Copy link
Contributor Author

OK, let's separate the bort testing from the documentation changes, in order to divide and conquer this PR. ;-)

I have put the tests (after fixing the ufbstp() seg faults) in new PR #422 .

Once those are merged I will circle around again to this PR. We will have a documentation meeting today to discuss some of the issues with the documentation changes in this PR.

@edwardhartnett
Copy link
Contributor Author

The testing work in the PR has been submitted in #422

We will discuss some documentation issues at today's meeting, and then I will resumbit some of the documentation work as future PRs...

@edwardhartnett edwardhartnett deleted the ejh_bort_more branch April 14, 2023 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants