Support collating sequence for indexed file keys of alphanumeric class #177

nberth · 2024-09-12T09:47:55Z

Warn when a collating sequence is ignored for keys of indexed files
Use file collating sequence for any key of alphanumeric class

Before this change, only keys that were alphanumeric elementary items or numeric display were supported.

GitMensch

minor changes requested, especially to the test cases and likely to a missing warning

cobc/tree.c

GitMensch · 2024-09-13T21:33:15Z

cobc/tree.c

@@ -4721,6 +4721,15 @@ validate_indexed_key_field (struct cb_file *f, struct cb_field *records,
 	}
 	k = CB_FIELD_PTR (key_ref);

+	/* check collating sequence is not ignored */
+	if (f->collating_sequence != NULL &&
+	    cbak == NULL &&	/* only if no alternate key is given (bit of hack for now) */


Why not checking the alternative keys in a loop as well? Aren't the collations used for those?
What about split keys?

Finding a way to avoid emitting "too much" warnings for the file-level collating sequence seems a little tedious here.
Split keys appear to always be handled as alphanumerics (as per the code).

tests/testsuite.src/syn_file.at

tests/testsuite.src/run_file.at

ddeclerck · 2024-09-26T11:46:28Z

Looks like this could be merged ; is anything missing here @GitMensch ?

GitMensch

Wondering: don't we need a --without-db and --with-vbisam setup in the workflow to verify the tests work as expected in those environments?

tests/testsuite.src/run_file.at

cobc/tree.c

tests/testsuite.src/syn_file.at

ddeclerck · 2024-09-27T08:15:38Z

Wondering: don't we need a --without-db and --with-vbisam setup in the workflow to verify the tests work as expected in those environments?

Last time I checked VBISAM with 3.x (was a while ago), there were testsuite failures.

GitMensch · 2024-09-27T08:22:26Z

Last time I checked VBISAM with 3.x (was a while ago), there were testsuite failures.

That would be unrelated to a --without-db --without-curses --without-xml2 --without-json build :-)

But even with a set of failures (even the NEWS file says that depending on the version of vbisam used there will be some unexpected passes and failures and will do so until we have a V-ISAM version released) this gets "only" down to working it out - we tinker the MSYS2 and MSVC builds for some testsuite expectations in GitHub CI as well - and this does help to catch any new issues that arise (and may also show some positive side effects we otherwise don't catch).
Note: I'm not planning to adjust any test expectations specific for VBISAM in GnuCOBOL 3.3 but the "no isam" and "not bdb" ones should definitely work.

GitMensch · 2024-09-27T08:24:05Z

I'm taking on the "minimal build" and send a PR for that (this should pass here in any case) --> #184 and would let the vbisam part optional to you.

ddeclerck · 2024-09-27T08:24:38Z

Last time I checked VBISAM with 3.x (was a while ago), there were testsuite failures.

That would be unrelated to a --without-db --without-curses --without-xml2 --without-json build :-)

Sure, but you mentionned --with-vbisam :-)

ddeclerck · 2024-09-30T15:18:28Z

I think Nicolas took care of all the requested changes ?
If so, is this OK to be merged @GitMensch ? (it's another burning issue for our customer)

GitMensch

Please adjust the testcases, then it is good to go (with the option to add a new warning group and/or national collation "up to libcob" now instead of later)

cobc/tree.c

GitMensch · 2024-09-30T18:40:50Z

cobc/tree.c

+			pos = key_ref;
+		}
+		if (colseq != NULL) {
+			cb_warning_x (COBC_WARN_FILLER, CB_TREE (pos),


Preferable would be a new warning group -Wignored-something enabled by default (and add the handful of "ignored" warnings to the same group, but as I currently don't have a clue for a good name and it isn't important this can be done in a follow-up commit :-)

May ignored-collations be too precise for this?

That would work but I'd prefer a warning option that we can use for any ignored elements (so yes, too specific ;-)

I see. So that'd be a category like ignored-file-control-clause or something even more general. I suggest to leave this for a subsequent PR (I won't be available in the very near future to work on that — currently in (pre-)holiday transit, somewhere in Europe ;).

Even more - totally global - so we can include every "ignored" that has no separate warning level (that should be 1 or two warnings currently).

And yes, that's for "sometime in the future".

cobc/codegen.c

tests/testsuite.src/run_file.at

Before this change, only keys that were alphanumeric elementary items or numeric display were supported.

…file

nberth · 2024-10-01T09:02:56Z

Now upstream.

nberth force-pushed the indexed-key-colseq-for-alphanum-groups branch from 2acff2b to b33b98e Compare September 12, 2024 09:50

nberth requested a review from GitMensch September 12, 2024 09:50

nberth force-pushed the indexed-key-colseq-for-alphanum-groups branch 2 times, most recently from 108a61b to 57b4fbc Compare September 12, 2024 10:11

GitMensch requested changes Sep 13, 2024

View reviewed changes

nberth force-pushed the indexed-key-colseq-for-alphanum-groups branch from 57b4fbc to 9be8442 Compare September 23, 2024 09:04

This comment was marked as outdated.

Sign in to view

nberth force-pushed the indexed-key-colseq-for-alphanum-groups branch 6 times, most recently from 511d25e to 56e6811 Compare September 24, 2024 09:59

nberth requested a review from GitMensch September 24, 2024 10:17

GitMensch requested changes Sep 26, 2024

View reviewed changes

tests/testsuite.src/run_file.at Outdated Show resolved Hide resolved

tests/testsuite.src/run_file.at Outdated Show resolved Hide resolved

cobc/tree.c Outdated Show resolved Hide resolved

tests/testsuite.src/syn_file.at Outdated Show resolved Hide resolved

nberth force-pushed the indexed-key-colseq-for-alphanum-groups branch from 2a365a5 to 0be00c2 Compare September 27, 2024 09:02

nberth requested a review from GitMensch September 27, 2024 09:39

GitMensch mentioned this pull request Sep 27, 2024

CI: adding minimal build #184

Merged

GitMensch requested changes Sep 30, 2024

View reviewed changes

nberth added 4 commits October 1, 2024 08:55

Warn when a collating sequence is ignored for keys of indexed files

f1d766e

Use file collating sequence for any key of alphanumeric class

e616365

Before this change, only keys that were alphanumeric elementary items or numeric display were supported.

Improve warning message for ignored collating sequences

f5bcc39

Adjust position of messages about unfinished KEY or FILE COLSEQs

bde91e1

nberth force-pushed the indexed-key-colseq-for-alphanum-groups branch from 0be00c2 to ca1b299 Compare October 1, 2024 07:13

nberth added 4 commits October 1, 2024 09:28

Turn non-BDB test for collation on group key into an expected failure

e548de3

Avoid new compile-time warnings in runtime test

3eca21a

Better guard for warnings about ignored collating sequences

5e63ce3

Preliminary codegen and test for NATIONAL SEQUENCE on key of indexed …

27797e8

…file

nberth force-pushed the indexed-key-colseq-for-alphanum-groups branch from ca1b299 to 27797e8 Compare October 1, 2024 07:41

GitMensch approved these changes Oct 1, 2024

View reviewed changes

GitMensch added the Ready for SVN label Oct 1, 2024

nberth closed this Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support collating sequence for indexed file keys of alphanumeric class #177

Support collating sequence for indexed file keys of alphanumeric class #177

nberth commented Sep 12, 2024

GitMensch left a comment

GitMensch Sep 13, 2024

nberth Sep 24, 2024

This comment was marked as outdated.

ddeclerck commented Sep 26, 2024

GitMensch left a comment

ddeclerck commented Sep 27, 2024

GitMensch commented Sep 27, 2024

GitMensch commented Sep 27, 2024 •

edited

Loading

ddeclerck commented Sep 27, 2024

ddeclerck commented Sep 30, 2024

GitMensch left a comment

GitMensch Sep 30, 2024

nberth Oct 1, 2024

GitMensch Oct 1, 2024

nberth Oct 1, 2024

GitMensch Oct 1, 2024

nberth commented Oct 1, 2024

Support collating sequence for indexed file keys of alphanumeric class #177

Support collating sequence for indexed file keys of alphanumeric class #177

Conversation

nberth commented Sep 12, 2024

GitMensch left a comment

Choose a reason for hiding this comment

GitMensch Sep 13, 2024

Choose a reason for hiding this comment

nberth Sep 24, 2024

Choose a reason for hiding this comment

This comment was marked as outdated.

ddeclerck commented Sep 26, 2024

GitMensch left a comment

Choose a reason for hiding this comment

ddeclerck commented Sep 27, 2024

GitMensch commented Sep 27, 2024

GitMensch commented Sep 27, 2024 • edited Loading

ddeclerck commented Sep 27, 2024

ddeclerck commented Sep 30, 2024

GitMensch left a comment

Choose a reason for hiding this comment

GitMensch Sep 30, 2024

Choose a reason for hiding this comment

nberth Oct 1, 2024

Choose a reason for hiding this comment

GitMensch Oct 1, 2024

Choose a reason for hiding this comment

nberth Oct 1, 2024

Choose a reason for hiding this comment

GitMensch Oct 1, 2024

Choose a reason for hiding this comment

nberth commented Oct 1, 2024

GitMensch commented Sep 27, 2024 •

edited

Loading