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

sbelondr · 2023-03-07T08:40:41Z

I propose the following fixes for the bugs :

#831 (Problem for the copy replacing in listing mode) ;
#863 (Variable is not referenced in report).

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) :

COPY replacement with multiple partial matches ;
COPY replacement with partial match.

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 · 2023-03-07T14:42:10Z

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).

GitMensch · 2023-03-07T16:35:50Z

cobc/cobc.c

@@ -6619,8 +6675,10 @@ print_line (struct list_files *cfile, char *line, int line_num, int in_copy)
 	struct list_skip	*skip;
 	int	do_print;
 	int	on_off;
+	char	pch;


please leave local defintions in, we try to limit scope where possible

GitMensch · 2023-03-07T16:38:13Z

cobc/cobc.c


 	cmp_line[0] = 0;
+	bzero (cmp_line, CB_LINE_LENGTH + 2);


From its manpage:

The bzero() function is deprecated (marked as LEGACY in POSIX.1-2001); use memset(3) in new programs. POSIX.1-2008 removes the specification of bzero().

The best option is to do it before the caller:

char cmp_line[CB_LINE_LENGTH + 2] = { 0 };

GitMensch · 2023-03-07T16:43:25Z

cobc/cobc.c

-				make_new_continuation_line (cfile_name, pline,
-							    pline_cnt, line_num);
-			}
+	// calculation the len char


Please use C89 comments here and in other places

- // calculation the len char + /* calculation the len char */

GitMensch · 2023-03-07T16:50:42Z

cobc/cobc.c

@@ -7393,6 +7214,43 @@ cleanup_copybook_reference (struct list_files *cur)
 	cobc_free (cur);
 }

+static
+char *get_word (char *str)


please move those two after get_next_nonspace to group those functions

GitMensch · 2023-03-07T16:52:21Z

cobc/cobc.c

-					print_line (cfile, pline[i], line_num + i, 0);
-					pline[i][0] = 0;
-				}
+				struct list_files 	*cur;


please combine the assignment into this line - nice scoping btw :-)

GitMensch · 2023-03-07T16:58:27Z

cobc/cobc.c

+	const int	margin_a = cobc_get_margin_a (1);
+	int	cnt_space = 0;
+	int	j = 0;
+


Looks like most of DEBUG_REPLACE is now out, was this explicit planned?
Did you use that to get an idea where the issue is?

ping @sbelondr

GitMensch

Please split this PR into two ones, as mentioned I think the one for the report stuff # 863 should be quite easy to get through.

We can keep this PR for # 831; so far I did make some minor review comments and get a general overview (not there yet, need to combe back to this later...); I see that the issues we had - marked as AT_XFAIL_IF (true) are solved, as you've mentioned one of the older REPLACE tests now fail, mostly because the replacement is done by starting on the next line instead of the original one. Can you adjust that?

sbelondr · 2023-03-08T09:10:57Z

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

GitMensch

please check the first review parts and consider handling the new cb_partial_replace_when_literal_src setting (otherwise thi can be included later)

please also fix the non-C89 definition - see the results of the new CI check (in detail here: https://github.com/OCamlPro/gnucobol/actions/runs/5392577360/jobs/9791122631#step:8:125)

I'll the review the result with a local checkout; the diff looks huge (and bigger as it is, while it is still big)

cobc/ChangeLog

cobc/tree.h

GitMensch · 2023-06-27T17:03:32Z

tests/ChangeLog

@@ -761,7 +767,7 @@
 	* testsuite.src/*.at: Added check for cobc's exit code and stderr
 		where missing

-2014-14-04  Philipp B�hme <[email protected]>
+2014-14-04  Philipp B�hme <[email protected]>


somethings broken here, maybe you can revert the line (otherwise I can do it after merging)

I'll take care of that in the next commit

GitMensch · 2023-06-27T17:08:03Z

cobc/cobc.c

+	if (!str) {
+		return NULL;
+	}
+	while (str[i] && isspace (str[i])) {


What about separator commas and separator semicolons?
The old code used isspace ((unsigned char)*bp) || *bp == ',' || *bp == '.' || *bp == ';'.

I also put that before but when I read the documentation I didn't understand why these characters is understood as a space and this not change the result of the test. I'll try other test for compare old replacing with the new replacing for check the differences

Note: it obviously would be good to have this in the testsuite, too.
In effect the point is that there are multiple "separators" - all "space characters", newline, separator semicolon and separator comma.
The last one is a bit tricky as, depending on DECIMAL-POINT IS COMMA this may or may not mean that "3,2" has to be separated (=2 "words"/tokens), while "3,a" would always mean two tokens. [note: the listing may "don't care" about that at all or always ignore the comma if DECIMAL-POINT IS COMMA is in effect].

I think those testcases are missing, right?

cobc/cobc.c

codecov-commenter · 2023-06-28T19:06:10Z

Codecov Report

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

❗ 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%)`	⬇️
libcob/common.c	`56.62% <40.00%> (-0.02%)`	⬇️
cobc/pplex.l	`72.26% <56.19%> (-0.26%)`	⬇️
cobc/ppparse.y	`61.96% <66.66%> (+0.81%)`	⬆️
cobc/parser.y	`68.55% <71.48%> (-0.12%)`	⬇️
cobc/cobc.c	`73.32% <79.38%> (+2.04%)`	⬆️
cobc/typeck.c	`64.81% <83.60%> (-0.13%)`	⬇️
cobc/codegen.c	`75.33% <84.93%> (+0.12%)`	⬆️
cobc/replace.c	`95.26% <95.26%> (ø)`
cobc/codeoptim.c	`27.44% <100.00%> (+0.56%)`	⬆️
... and 2 more

... and 2 files with indirect coverage changes

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

GitMensch · 2023-08-23T11:37:37Z

@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)

GitMensch · 2023-08-23T11:42:17Z

cobc/cobc.c

+	while (line[i])
+	{
+		/* loop space */
+		while (line[i] && isspace (line[i]) && !is_quotes_char (line[i], &simple_quote, &double_quote) && NOT_TOO_LONG (i, start)) {


We have isspace (line[i]), so how should be (line[i]) be a quote?

Yes, it's useless. I removed that

GitMensch · 2023-08-23T11:49:28Z

cobc/cobc.c

@@ -6741,6 +6783,146 @@ print_fixed_line (const int line_num, char pch, char *line)
 	}
 }

+static int


Please add a doc comment before the function so it is clear what is checks/changes.

Also: does the code handle an "escaped "" quote like in x""4100"""?

I added a comment for this function but I didn't understand this format x""4100"".

I think that I'm not handling this but I can't find this in the old code.

Is this "translated" in the code to something else ?

Sorry for this ping @GitMensch, can I have more information on this ?

add fix for the tickets #831 and #863

1ba2da0

GitMensch reviewed Mar 7, 2023

View reviewed changes

GitMensch requested changes Mar 7, 2023

View reviewed changes

sbelondr added 2 commits April 26, 2023 17:43

remove patch #863 and fix bug listing in last test

bd52a86

Merge branch 'gcos4gnucobol-3.x' into gcos4gnucobol-3.x

c7a4a92

GitMensch changed the title ~~Fix for the svn tickets #831 and #863~~ Fix for bug #831: COPY REPLACING in listing Jun 13, 2023

Merge branch 'gcos4gnucobol-3.x' into gcos4gnucobol-3.x

c874ce5

GitMensch requested changes Jun 27, 2023

View reviewed changes

fix syntax and delete a check

8bc8b6c

lefessan mentioned this pull request Jul 5, 2023

Fix COPY REPLACING and REPLACE #75

Closed

sbelondr and others added 8 commits July 20, 2023 15:11

edit function check simple and double quotes

dafd128

fix conflit

43d0968

fix error in the listing mode

67e9554

remove patch #863 and fix bug listing in last test

edcae73

fix syntax and delete a check

e20cf1c

edit function check simple and double quotes

f669da3

resolve conflit

f7e680e

Merge branch 'gcos4gnucobol-3.x' into gcos4gnucobol-3.x

4c5191d

GitMensch reviewed Aug 23, 2023

View reviewed changes

add comment and remove expected.lst in the tests

2bd81c1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sbelondr commented Mar 7, 2023

GitMensch commented Mar 7, 2023 •

edited

Loading

GitMensch Mar 7, 2023

GitMensch Mar 7, 2023 •

edited

Loading

GitMensch Mar 7, 2023

GitMensch Mar 7, 2023

GitMensch Mar 7, 2023

GitMensch Mar 7, 2023

GitMensch Aug 23, 2023

GitMensch left a comment

sbelondr commented Mar 8, 2023

GitMensch left a comment •

edited

Loading

GitMensch Jun 27, 2023

sbelondr Jun 28, 2023

GitMensch Jun 27, 2023

sbelondr Jun 28, 2023

GitMensch Jun 28, 2023

GitMensch Aug 23, 2023

codecov-commenter commented Jun 28, 2023 •

edited

Loading

GitMensch commented Aug 23, 2023

GitMensch Aug 23, 2023 •

edited

Loading

sbelondr Sep 7, 2023

GitMensch Aug 23, 2023

sbelondr Sep 7, 2023

sbelondr Oct 11, 2023

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

Are you sure you want to change the base?

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

Conversation

sbelondr commented Mar 7, 2023

GitMensch commented Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

GitMensch Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitMensch left a comment

Choose a reason for hiding this comment

sbelondr commented Mar 8, 2023

GitMensch left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 28, 2023 • edited Loading

Codecov Report

GitMensch commented Aug 23, 2023

GitMensch Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitMensch commented Mar 7, 2023 •

edited

Loading

GitMensch Mar 7, 2023 •

edited

Loading

GitMensch left a comment •

edited

Loading

codecov-commenter commented Jun 28, 2023 •

edited

Loading

GitMensch Aug 23, 2023 •

edited

Loading