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 21 forks source link

Allow specifying a file as argument in -febcdic-table #78

Closed ddeclerck closed 1 year ago

ddeclerck commented 1 year ago

TODO: add a test

@GitMensch Would something like this work for you ?

Alternatively, I could make a single function combining cob_load_collation and cob_get_collation_by_name, but that could cause confusion since the tables would be dynamically allocated when loading from a file but not when copying pointers to builtin tables.

Note that, even though the external table is built into the generated program, I made the function that reads the table available in libcob, so that external tools (GCSORT in my case) can use that too.

P.S: Should I add my EBCDIC500 to Latin-1 table somewhere in the repo ?

codecov-commenter commented 1 year ago

Codecov Report

Merging #78 (52e7502) into gcos4gnucobol-3.x (516b813) will increase coverage by 0.00%. The diff coverage is 70.32%.

@@                Coverage Diff                 @@
##           gcos4gnucobol-3.x      #78   +/-   ##
==================================================
  Coverage              65.27%   65.27%           
==================================================
  Files                     31       31           
  Lines                  56425    56442   +17     
  Branches               14723    14737   +14     
==================================================
+ Hits                   36830    36844   +14     
+ Misses                 13807    13804    -3     
- Partials                5788     5794    +6     
Impacted Files Coverage Δ
libcob/coblocal.h 100.00% <ø> (ø)
cobc/codegen.c 74.84% <42.85%> (+0.03%) :arrow_up:
cobc/cobc.c 70.99% <75.00%> (-0.02%) :arrow_down:
libcob/cconv.c 75.34% <75.00%> (-1.13%) :arrow_down:
cobc/flag.def 100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

nberth commented 1 year ago

P.S: Should I add my EBCDIC500 to Latin-1 table somewhere in the repo ?

Does this one correspond to the IBM table without irregularities (documented in a comment somewhere in cconv)?

ddeclerck commented 1 year ago

P.S: Should I add my EBCDIC500 to Latin-1 table somewhere in the repo ?

Does this one correspond to the IBM table without irregularities (documented in a comment somewhere in cconv)?

No, not really. The conversion table in cconv (and in the IBM documentation) actually maps EBCDIC 500 (yes, 500, not 37 nor 1047) characters to 7-bit ASCII. However, if we only consider the first 128 characters of Latin-1, then the table are almost identical, the only exception being EBCDIC 6A (¦) remapped to ASCII 7C (|), since ¦ does not exist in 7-bit ASCII, and as a consequence EBCDIC BB (|) replaced by the ASCII substitution character 1A (I wonder why they simply did not directly replace EBCDIC 6A (¦) with a substitution character...).

[the following is a bit off-topic, but out of curiosity]

Interestingly, the documentation about conversion irregularities (https://www.ibm.com/docs/en/iis/11.3?topic=tables-conversion-table-irregularities) seems to be using the EBCDIC 1047 representation of characters in the first column (i.e characters [ and ] at positions AD and BD). Yet it maps EBCDIC 4A and 5A to ASCII 5B and 5D ([ and ]), which suggests they intended to map EBCDIC 500 characters.

GitMensch commented 1 year ago

For the changelogs - and because we won't do things like that between rc-1 and rc-2/final otherwise: https://sourceforge.net/p/gnucobol/feature-requests/429/

GitMensch commented 1 year ago

Feel free to drop a note as soon as you consider this PR ready and I'll have a look.

ddeclerck commented 1 year ago

@GitMensch not done yet

ddeclerck commented 1 year ago

@GitMensch I think everything has been addressed.

GitMensch commented 1 year ago

After handling some relative minor issues, I consider the PR ready for check in to 3.x branch now, reference the commit in the related FR and close that.

ddeclerck commented 1 year ago

@GitMensch Merged on SVN. I'm now going to fix GCSORT to use these changes.

GitMensch commented 1 year ago

Note: because you made me check VS2008 for GCSORT I also did so for 3.x - isblank is not available there, I've replaced it by the more matching isspace.

Again: Thanks for the changes (and also thanks for caring for GCSORT). It is also good to know that the collation issues work for your conversion project now.