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

Fix for bug #831: COPY REPLACING in listing #92

Open sbelondr opened 1 year ago

sbelondr commented 1 year ago

Hi @GitMensch,

I propose the following fixes for the bugs :

For the bug #863, I just add a loop to check if the variable exist (codegen.c). I added a test in run_reportwriter.at.

For the bug #831 I rewrite partialy the listing mode in the cobc.c file and I add 2 struct in tree.h for manage lines and replacements.

I added a test in the listings.at file (name : Multiple replacement) and I completed two tests (just add the expect.lst file) :

But I didn't pass the huge REPLACE test because I add more space of the default implementation (8 precisely).

I would like to know if the implementation is right for you (I remove backtracking) ?

GitMensch commented 1 year ago

Thanks you for working on this. Note that two PRs, one for each change are commonly more easy to review.

Let's start with the most easiest one: 863. Please adjust the check to be done in finalize_report, changing Control field %s is not referenced in report to a warning (the printf is wrong and codegen is too late in general). It is fine to set the control field to NULL there if we don't need it in other places (the only one that comes to mind would be the symbol listing), which allows to drop the check you've added to codegen and the old check, that raised the warning via printf.

Please move also the other code fro codegen.c with printf to finalize_report and change it to raise a warning:

    if((f->report_flag & COB_REPORT_LINE)
    && f->children
    && (f->children->report_flag & COB_REPORT_LINE)) {
        printf("Warning: Ignoring nested LINE %s %d\n",
            (f->report_flag & COB_REPORT_LINE_PLUS)?"PLUS":"",
            f->report_line);
        f->report_line = 0;
        f->report_flag &= ~COB_REPORT_LINE_PLUS;
        f->report_flag &= ~COB_REPORT_LINE;
    }

Both places can use cb_warning (COBC_WARN_FILLER, _(warning_msg_here)); - or cb_warning_x if this is more reasonable to get the right warning position.

A PR with those adjustments can be reviewed quickly and would definitely be fine for inclusion into the 3.x branch before the upcoming RC3 (roughly planned for end of the week, maybe a bit earlier).

sbelondr commented 1 year ago

Thank you for the feedback. I'm working on it.

codecov-commenter commented 1 year ago

Codecov Report

Merging #92 (f7e680e) into gcos4gnucobol-3.x (743651f) will increase coverage by 0.27%. The diff coverage is 79.46%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@                  Coverage Diff                  @@
##           gcos4gnucobol-3.x      #92      +/-   ##
=====================================================
+ Coverage              65.19%   65.46%   +0.27%     
=====================================================
  Files                     31       32       +1     
  Lines                  58363    58895     +532     
  Branches               15377    15514     +137     
=====================================================
+ Hits                   38050    38558     +508     
+ Misses                 14382    14316      -66     
- Partials                5931     6021      +90     
Impacted Files Coverage Δ
cobc/scanner.l 75.44% <38.46%> (-0.40%) :arrow_down:
libcob/common.c 56.62% <40.00%> (-0.02%) :arrow_down:
cobc/pplex.l 72.26% <56.19%> (-0.26%) :arrow_down:
cobc/ppparse.y 61.96% <66.66%> (+0.81%) :arrow_up:
cobc/parser.y 68.55% <71.48%> (-0.12%) :arrow_down:
cobc/cobc.c 73.32% <79.38%> (+2.04%) :arrow_up:
cobc/typeck.c 64.81% <83.60%> (-0.13%) :arrow_down:
cobc/codegen.c 75.33% <84.93%> (+0.12%) :arrow_up:
cobc/replace.c 95.26% <95.26%> (ø)
cobc/codeoptim.c 27.44% <100.00%> (+0.56%) :arrow_up:
... and 2 more

... and 2 files with indirect coverage changes

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

GitMensch commented 1 year ago

@sbelondr: please adjust the testsuite changes - we moved from separate "expected.lst" to "list in stdout", so the changed expectation must be adjusted there (you currently have it just added as new AT_DATA)