Handle BDB keys of different length #156

engboris · 2024-07-09T12:58:10Z

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.

ddeclerck · 2024-07-09T17:50:14Z

libcob/fileio.c

@@ -919,20 +919,34 @@ bdb_bt_compare (DB *db, const DBT *k1, const DBT *k2
 )
 {
 	const unsigned char *col = (unsigned char *)DBT_GET_APP_DATA (k1);
+#ifdef USE_BDB_KEYDIFF /* flag passed with CPPFLAGS */
+	COB_MODULE_PTR->collating_sequence = col;


This changes the current module's collating sequence; we don't want that.

agreed, we also don't want cob_fields to be used here - so extract the logic that only operates on data + size + col from cob_cmp_alnum and locally export it, then calling this here

That's still open... just drop that line

GitMensch

the main open point is the previously unclear registration:

when USE_BDB_KEYDIFF is used then bdb_bt_compare needs to be always registered (it currently is only registered if there is a col in use)

For your example: parameters should be passed as arguments to configure, instead of being set inline/exported (if you are interested why: the autotools manual has an entry on that, but the biggest part is that passed arguments are checked to be identical between runs).

libcob/common.h

GitMensch · 2024-07-09T17:53:46Z

libcob/fileio.c

@@ -919,20 +919,34 @@ bdb_bt_compare (DB *db, const DBT *k1, const DBT *k2
 )
 {
 	const unsigned char *col = (unsigned char *)DBT_GET_APP_DATA (k1);
+#ifdef USE_BDB_KEYDIFF /* flag passed with CPPFLAGS */
+	COB_MODULE_PTR->collating_sequence = col;


agreed, we also don't want cob_fields to be used here - so extract the logic that only operates on data + size + col from cob_cmp_alnum and locally export it, then calling this here

libcob/fileio.c

GitMensch

Please adjust as noted in the fix-commit (I've screwed up and commented there instead of on the PR).

In any case you want to further add to the ChangeLog.

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.

In any case this can be done in a later refactoring on as well.

engboris · 2024-07-10T13:30:38Z

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 · 2024-07-10T16:12:34Z

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 · 2024-07-10T16:47:36Z

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 · 2024-07-10T17:29:53Z

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 */.

GitMensch

some finalization, then ready to check-in upstream

GitMensch · 2024-07-10T17:31:35Z

libcob/common.c

+	return cob_cmp_strings ((unsigned char *)data1, (unsigned char *)data2,
+		size1, size2, col);


Is there a reason to not pass the arguments directly without intermediate variables?

@engboris you've marked that as solved but the question is unanswered?

GitMensch · 2024-07-10T17:32:54Z

libcob/fileio.c

@@ -919,20 +919,34 @@ bdb_bt_compare (DB *db, const DBT *k1, const DBT *k2
 )
 {
 	const unsigned char *col = (unsigned char *)DBT_GET_APP_DATA (k1);
+#ifdef USE_BDB_KEYDIFF /* flag passed with CPPFLAGS */
+	COB_MODULE_PTR->collating_sequence = col;


That's still open... just drop that line

GitMensch · 2024-07-10T17:33:24Z

libcob/fileio.c

+	unsigned char *data1 = k1->data;
+	unsigned char *data2 = k2->data;
+	size_t size1 = (size_t)k1->size;
+	size_t size2 = (size_t)k2->size;
+	return cob_cmp_strings (data1, data2, size1, size2, col);


Is there a reason for intermediate variables here?

There is no reason. It was temporary and they were meant to be removed.

you've removed those here, but left them in line 2031++ https://github.com/OCamlPro/gnucobol/pull/156/files#diff-16de7d2ecd4918cd696dff002aebea147395b0d09288c2e8e0310155dc943da1R2031-R2037

libcob/fileio.c

libcob/ChangeLog

GitMensch

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

GitMensch · 2024-07-11T08:09:53Z

libcob/common.c

+	return cob_cmp_strings ((unsigned char *)data1, (unsigned char *)data2,
+		size1, size2, col);


@engboris you've marked that as solved but the question is unanswered?

libcob/common.c

engboris · 2024-07-11T08:29:24Z

It should be good now!

codecov-commenter · 2024-07-11T08:31:49Z

⚠️ Please install the 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 ⚠️

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

Additional details and impacted files

@@                  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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chuck-haatvedt · 2024-07-12T02:01:13Z

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, - using the optional collating sequence of the program */ let's include the old doc comment, adjusted to "strings, space padded, using an optional collating sequence" here so this "local exported" function has a doc comment In libcob/common.c: + return cob_cmp_strings ((unsigned char *)data1, (unsigned char *)data2, + size1, size2, col); @engboris you've marked that as solved but the question is unanswered? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***> [ { ***@***.***": "http://schema.org", ***@***.***": "EmailMessage", "potentialAction": { ***@***.***": "ViewAction", "target": "#156 (review)", "url": "#156 (review)", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { ***@***.***": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

ddeclerck · 2024-07-12T09:03:03Z

Merged in SVN @ 5296.

engboris added the bug Something isn't working label Jul 9, 2024

engboris requested review from GitMensch and ddeclerck July 9, 2024 12:58

ddeclerck reviewed Jul 9, 2024

View reviewed changes

GitMensch requested changes Jul 9, 2024

View reviewed changes

engboris requested a review from GitMensch July 10, 2024 12:14

GitMensch requested changes Jul 10, 2024

View reviewed changes

engboris force-pushed the bdb-compare-keys-diff-length branch from 2408d79 to 986ffd8 Compare July 10, 2024 13:21

engboris changed the title ~~[WIP] Handle BDB keys of different length~~ Handle BDB keys of different length Jul 10, 2024

GitMensch requested changes Jul 10, 2024

View reviewed changes

engboris force-pushed the bdb-compare-keys-diff-length branch from 42bb2de to 373e465 Compare July 11, 2024 07:56

GitMensch requested changes Jul 11, 2024

View reviewed changes

engboris force-pushed the bdb-compare-keys-diff-length branch from 59f5c22 to 62f1253 Compare July 11, 2024 08:26

engboris force-pushed the bdb-compare-keys-diff-length branch from 6c83450 to 4430207 Compare July 11, 2024 08:47

GitMensch added the Ready for SVN label Jul 11, 2024

GitMensch approved these changes Jul 11, 2024

View reviewed changes

Handle BDB keys of different length

aa3fc1a

ddeclerck added the Merged in SVN label Jul 12, 2024

ddeclerck force-pushed the bdb-compare-keys-diff-length branch from 4430207 to aa3fc1a Compare July 12, 2024 09:02

ddeclerck closed this Jul 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle BDB keys of different length #156

Handle BDB keys of different length #156

engboris commented Jul 9, 2024 •

edited

Loading

ddeclerck Jul 9, 2024

GitMensch Jul 9, 2024

GitMensch Jul 10, 2024

GitMensch left a comment

GitMensch Jul 9, 2024

GitMensch left a comment

engboris commented Jul 10, 2024 •

edited

Loading

GitMensch commented Jul 10, 2024

engboris commented Jul 10, 2024 •

edited

Loading

GitMensch commented Jul 10, 2024

GitMensch left a comment

GitMensch Jul 10, 2024

GitMensch Jul 11, 2024

GitMensch Jul 10, 2024

GitMensch Jul 10, 2024

engboris Jul 11, 2024

GitMensch Jul 11, 2024

GitMensch left a comment

GitMensch Jul 11, 2024

engboris commented Jul 11, 2024

codecov-commenter commented Jul 11, 2024

chuck-haatvedt commented Jul 12, 2024 via email

ddeclerck commented Jul 12, 2024

		return cob_cmp_strings ((unsigned char )data1, (unsigned char )data2,
		size1, size2, col);

Handle BDB keys of different length #156

Handle BDB keys of different length #156

Conversation

engboris commented Jul 9, 2024 • edited Loading

Problem

Solution

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitMensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitMensch left a comment

Choose a reason for hiding this comment

engboris commented Jul 10, 2024 • edited Loading

GitMensch commented Jul 10, 2024

engboris commented Jul 10, 2024 • edited Loading

GitMensch commented Jul 10, 2024

GitMensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitMensch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engboris commented Jul 11, 2024

codecov-commenter commented Jul 11, 2024

Codecov Report

chuck-haatvedt commented Jul 12, 2024 via email

ddeclerck commented Jul 12, 2024

engboris commented Jul 9, 2024 •

edited

Loading

engboris commented Jul 10, 2024 •

edited

Loading

engboris commented Jul 10, 2024 •

edited

Loading