OCamlPro / gnucobol

A clone of the sourceforge GnuCOBOL compiler from COBOL to C.
https://get-superbol.com
GNU Lesser General Public License v3.0
16 stars 20 forks source link

Handle BDB keys of different length #156

Closed engboris closed 3 weeks ago

engboris commented 3 weeks ago

Fix for issue #154

Problem

The function libcob/fileio.c:bdb_bt_compare fails on keys of different length passed in arguments. This is necessary since valid BDB files containing additional data interpreted as keys of different length may be used with GnuCOBOL (example given from one of our customers). Such files may be produced by external tools but still meant to be readable.

Solution

Use the already implemented libcob/common.c:cob_cmp_alnum function (now made external) to compare keys as alphanumeric fields of possibly different length (padded with spaces).

This PR introduces a new preparser flag USE_BDB_KEYDIFF with may be passed in the following way when compiling GnuCOBOL sources:

CPPFLAGS=-DUSE_BDB_KEYDIFF ./configure
make

This flag allows such BDB keys of different length as an option since it has an overhead cost.

Notes

I have also been suggested to

also include the variant that operates on not 100% confirming BDB files (always using a sort function) (from @GitMensch, cf. issue #154)

Which is something I do not currently understand.

engboris commented 3 weeks ago

One thing I do wonder: should cob_cmp_strings() have a parameter int national and do a different compare for the spaces in this case? cob_cmp_alnum() would pass zero, bdb_bt_compare() would pass depending on the national flag in k1.

It sounds right to me. I've looked at the DBT structure and in particular its flags attribute but I don't see anything corresponding to a national flag

GitMensch commented 3 weeks ago

It sounds right to me. I've looked at the DBT structure and in particular its flags attribute but I don't see anything corresponding to a national flag

That's not inside of the DBT->flags (those really belong to BDB), but instead it is inside the cob_field, which only is used to set the DBT->data+size (see DBT_SET macro); we could change the appdata to be a struct, but then: this isn't really useful as we'd check this each time. The much more easier solution is to register either bdb_bt_compare or bdb_bt_compare_nat, as we do have this information when checking f->keys[i] there.

engboris commented 3 weeks ago

It might be wiser for us to leave that for a later refactoring as there is a need from one of our customers and I'm not very familiar with character sets and national yet (I currently don't know what this int national argument here would be). What do you think?

GitMensch commented 3 weeks ago

I agree, let's finish this PR just leaving a note at the place where we register the compare function, possibly something like /* TODO: add national compare function later */.

engboris commented 3 weeks ago

It should be good now!

codecov-commenter commented 3 weeks ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 66.11%. Comparing base (db7db96) to head (62f1253). Report is 39 commits behind head on gcos4gnucobol-3.x.

Files Patch % Lines
libcob/common.c 50.00% 0 Missing and 4 partials :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## gcos4gnucobol-3.x #156 +/- ## ===================================================== + Coverage 65.78% 66.11% +0.32% ===================================================== Files 32 33 +1 Lines 59416 60044 +628 Branches 15694 15802 +108 ===================================================== + Hits 39087 39698 +611 + Misses 14311 14295 -16 - Partials 6018 6051 +33 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

chuck-haatvedt commented 3 weeks ago
My final fix-commit is done now.
  This fixes the issue with a build without curses. It also fixes
  warnings in libcob related to C90 ==>
  declaration-after-statement.

  sorry about the delay...

    @page { margin: 0.79in }
    p { margin-bottom: 0.08in }

  Chuck
          Haatvedt
  ***@***.***

On 7/11/2024 3:13 AM, Simon Sobisch
  wrote:

  @GitMensch requested changes on this pull request.
  Apart from the unanswered answer (and the doc
    comment) that's finished; would you like to commit that upstream
    then?
    [note: upstream 3.x is currently failing CI, you may want to
    wait until @chuck-haatvedt did his final
    fix-commit tomorrow

  In libcob/common.c:
  > -/* compare content of field 'f1' to content of 'f2', space padded,
ddeclerck commented 3 weeks ago

Merged in SVN @ 5296.