NATIONAL + UTF8 tests, ref-mod: fix byte offset/size with poor man's display conversion#280
NATIONAL + UTF8 tests, ref-mod: fix byte offset/size with poor man's display conversion#280GitMensch wants to merge 2 commits intoOCamlPro:gitside-gnucobol-3.xfrom
Conversation
GitMensch
left a comment
GitMensch
left a comment
There was a problem hiding this comment.
Thank you for this first iteration - looks good!
We definitely need a Changelog entry in cobc and libcob, so please add those in your next commit.
| if (CB_TREE_CLASS (x) == CB_CLASS_NATIONAL) { | ||
| id = lookup_attr (COB_TYPE_NATIONAL, 0, 0, 0, NULL, 0); | ||
| } else { | ||
| id = lookup_attr (COB_TYPE_ALPHANUMERIC, 0, 0, 0, NULL, 0); | ||
| } |
There was a problem hiding this comment.
That's correct. Something similar may be needed for the national literals (then to be added directly above).
libcob/termio.c
Outdated
| /* poor man's conversion */ | ||
| if (COB_FIELD_IS_NATIONAL (f)) { | ||
| size_t i; | ||
| for (i = 0; i < f->size; i += 2) { | ||
| if (f->data[i] == 0x00) { | ||
| putc (f->data[i + 1], fp); | ||
| } | ||
| } | ||
| return; | ||
| } | ||
| display_alnum (f, fp); |
There was a problem hiding this comment.
add a new display_national function and use an if/else to get the right one
| if (COB_MODULE_PTR->flag_pretty_display) { | ||
| pretty_display_numeric ((cob_field *)f, fp); | ||
| return; | ||
| } | ||
| display_numeric ((cob_field *)f, fp); |
There was a problem hiding this comment.
you also can have national numeric data:
01 SOME-NUM-NAT PIC 9(6)v99 USAGE NATIONAL VALUE 12.34.
Therefore those two functions will possibly need handling of the national attribute as well (all data would be expected to be 0x00 = ISO8859-1 in the first byte ... but in this case it would be best to do a conversion to an internal alphanumeric field by skipping the low-value in the lower nibble and display it afterwards [for numerics that's fine as we can use a small fixed buffer - in case of national "text" we may get 2 GB fields so definitely don't want to do a conversion with a temporary field on that),
With the temporary alpahnumeric field we can simple call the normal display_numeric functions.
Can you please add a test of
DISPLAY SOME-NUM-NAT. (compiled with -fpretty-display/-fno-pretty-display)
(either fix the failing result with the approach above or mark it as expected fail)
tests/testsuite.src/run_refmod.at
Outdated
| PROGRAM-ID. prog. | ||
| DATA DIVISION. | ||
| WORKING-STORAGE SECTION. | ||
| 01 X PIC U(4) VALUE U"aǭcde". |
There was a problem hiding this comment.
use a single-byte here - that should pass (we have the mixed-width test below)
There was a problem hiding this comment.
as this file had no 2025 changes and yours are the first in 2026: please add , 2026 the the years in the header
There was a problem hiding this comment.
same for data_display.at
| size_t i; | ||
| for (i = 0; i < f->size; i += 2) { | ||
| if (f->data[i] == 0x00) { | ||
| putc (f->data[i + 1], fp); | ||
| } | ||
| } |
There was a problem hiding this comment.
please use the same pointer arithmetic (just with p += 2) and the temporary const int as well as the check we have in display_alnum; background: putc may return an error in which case we should break out of the loop instead of creating more errrors
| static void | ||
| display_national (const cob_field *f, FILE *fp) | ||
| { | ||
| size_t i; |
There was a problem hiding this comment.
please add a /* TODO */ marker here outlining that we currently only display national data that overlaps ISO8859-1 and will need an iconv approach later on
| @@ -1 +1 @@ | |||
| ## Copyright (C) 2003-2012, 2014-2015, 2017-2020, 2013 Free Software Foundation, Inc. | |||
There was a problem hiding this comment.
Should be
2003-2012, 2014-2015, 2017-2020, 2023, 2026 here
| ]) | ||
|
|
||
| AT_CLEANUP | ||
|
|
There was a problem hiding this comment.
please use two empty lines before a new test
| 2026-03-23 Preston Horne <preston.m.horne@vanderbilt.edu> | ||
|
|
||
| * testsuite.src/run_refmod.at: add static and dynamic | ||
| reference-modification tests for NATIONAL and UTF-8 fields | ||
| * testsuite.src/data_display.at: add DISPLAY test for NATIONAL | ||
| fields and literals with HEX-OF | ||
|
|
There was a problem hiding this comment.
not obvious:
this file is actually only adjusted for testcases if we heavily refactor them, otherwise this is left to "infrastucture" changes
--> nice (but not enforeced) to be part of the log message
| AT_CHECK([$COMPILE -Wno-unfinished prog.cob], [0], [], []) | ||
| AT_CHECK([$COBCRUN_DIRECT ./prog], [0], | ||
| [1234 | ||
| 0031003200330034 | ||
| 00001234 | ||
| 00300030003000300031003200330034 | ||
| abcd | ||
| 0061006200630064 | ||
| ]) |
There was a problem hiding this comment.
that's well, as this is displaying numerics, you may compile and run it twice: (with -fpretty-display/-fno-pretty-display) as this will go into different functions
(either fix the failing result with the approach mentioned in termio.c or keep it marked as expected fail)
2 participants
PR to review and discuss @hornepm's changes which serves also as a way to get a better picture what the GSoC proposal need to cover / the project will look like