OCamlPro / gnucobol

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

[GCOS] Add support for `L' character in picture strings #42

Closed nberth closed 2 years ago

nberth commented 2 years ago

For now, only runtime support for DISPLAY and MOVE is included.

GitMensch commented 2 years ago

@nberth: I understand the idea of this change, but the runtime part (and therefore the codegen) is a design break (not necessarily a show-stopper but a reason to tripple-check).

Would it be possible to do it similar to other OCCURS DEPENDING items?

01 tab.
   05 tab-row occurs 1 to 50 depending on some-var pic x.

In this case the codegen sets the field_size from some-var - the runtime doesn't need to know about its varying length.

Note: this only works if a MOVE some-string TO lvar does not change its size; something we actually want to add later (ISO COBOL DYNAMIC LENGTH fields - which has no explicit length set, but resets its length and storage depending on what is moved to it) - but I guess that L-PICs always have the length based on its depending fields - in which case its handling be made according to other OCCURS DEPENDING would be most reasonable.

nberth commented 2 years ago

Indeed I designed the current tentative draft with more generality than needed. It may be possible to reuse the OCCURS DEPENDING mechanics.

GitMensch commented 2 years ago

Did you have a look at using the OCCURS DEPENDING mechanics?

Note: I think there should be an option to the right to define this PS as work-in-progress/draft. Using this also helps to directly see which PRs may be inspected.

GitMensch commented 2 years ago

you may want to change this and other WIP but the OCamlPro-GCOS-Full one to a draft. This will show in the UI, so easier recognizable and won't trigger CI if not explicit requested via action.

nberth commented 2 years ago

@GitMensch So in the end I think I've managed to use the OCCURS DEPENDING mechanics.

Supporting the possible combinations of variable-length fields (PIC L…) and REDEFINES led me to implement a simplification in codegen.c (output_base) that may be tricky to validate; however all the tests seem to pass for now.

codecov-commenter commented 2 years ago

Codecov Report

Merging #42 (930bfb4) into gcos4gnucobol-3.x (e07100c) will increase coverage by 0.06%. The diff coverage is 75.42%.

@@                  Coverage Diff                  @@
##           gcos4gnucobol-3.x      #42      +/-   ##
=====================================================
+ Coverage              66.12%   66.18%   +0.06%     
=====================================================
  Files                     32       32              
  Lines                  52144    52204      +60     
  Branches               13484    13508      +24     
=====================================================
+ Hits                   34480    34552      +72     
+ Misses                 12232    12221      -11     
+ Partials                5432     5431       -1     
Impacted Files Coverage Δ
cobc/codegen.c 72.37% <70.00%> (+0.02%) :arrow_up:
cobc/field.c 68.45% <70.58%> (+0.17%) :arrow_up:
cobc/parser.y 69.32% <70.58%> (+0.09%) :arrow_up:
cobc/tree.c 74.13% <79.41%> (+0.08%) :arrow_up:
cobc/typeck.c 64.04% <81.81%> (+0.09%) :arrow_up:
cobc/config.def 100.00% <100.00%> (ø)
libcob/move.c 58.31% <0.00%> (+0.90%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us.

nberth commented 2 years ago

@GitMensch I'm getting unexpected results on GCOS, notably for the INITIALIZE statement (that seems to obey the DEPENDING, contrary to what the documentation seems to indicate). So I'll do some more tests to check.

nberth commented 2 years ago

Otherwise, when the DEPENDING var <= 1 >= max, we get errors at runtime by default (we don't really know if/how runtime check may be disabled).

GitMensch commented 2 years ago

Otherwise, when the DEPENDING var <= 1 >= max, we get errors at runtime by default (we don't really know if/how runtime check may be disabled).

I guess there is a runtime error with GnuCOBOL already when using --debug - right? If missing please add a testcase for this.

nberth commented 2 years ago

I've updated the NEWS file to mention the specific support for PICTURE L, and took the occasion to mention the CONTROL DIVISION that, albeit being quite obsolete, seems very specific to GCOS and may still be found in various code bases.

GitMensch commented 2 years ago

Congrats - this is nearly finished. Please handle #55 -> upstream, then update from upstream, adjust the msgids and we're good.

nberth commented 2 years ago

now upstream