Hello,
I've fixed ticket number #778. For that I've edited pplex.l (ppecho).
The problem comes from the spaces remove for queue->text.
You can find a test case in the patch.
Thank you very much for this well formed and complete patch!
Looks good. As I'm not sure about the initial idea of the removed code I've checked svn blame, which showed the code you've removed for the fix was already in OC 1.0.
From "looking" at it it possibly is intended to strip away the line breaks and newlines from the queue (but it doesn't, it just checks the first byte, in which case it may would be possible to not add it to the queue in the first case).
@sbelondr please don't take this as offending, it is just a "core" place in our replacing code that obviously "worked" for all cases but this one since nearly 18 years... It is also undocumented and .. special.
I hope @knishida may have a look at this one. Nonetheless I'll very likely include this change end of next week (to be part of 3.2rc1).
Last edit: Simon Sobisch 2021-12-16
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi @sf-mensch,
A team encounters some problem with this patch. I will perform other tests to check if it comes from this patch.
I would come back here later.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi,
I publish new patch because the last fix could cause a new error because it didn't check the other values to be replaced. I've provided a code to test.
I think that the while loop I removed matched this line of the MF (chapter REPLACE Statement, rule 2) : For purposes of matching, each occurrence of a separator comma, semicolon or space in pseudo-text-1 or in the source program text is considered to be a single space. Each sequence of one or more space separators is considered to be a single space.
For the new patch :
The problem with the replacing algo (ppecho function) is that it doesn't check the values correctly if there was a Partial match. For example :
copy.inc:
02 TEST-VAR PIC X(2) VALUE "OK".
02 TEST-OK PIC X(6) VALUE "OK BIS".
main.cob:
COPY "copy.inc" REPLACING LEADING ==TEST-VAR== BY ==TEST-AVR==
== 02 TEST-OK == BY == 02 TEST-KO ==.
When we have that, cobc handles it like this:
For this line: 02 TEST-VAR PIC X(2) VALUE "OK". :
Search with 02 -> no match for TEST-VAR, partial match for 02 TEST-OK
Search with 02 TEST-VAR -> no match for TEST-VAR and 02 TEST-OK
Search with PIC -> ...
So during a Partial match, the function doesn't test on the next values (TEST-VAR) and goes to the next token (PIC).
I've added a loop to check all the words (queue) when it doesn't match after a Partial match.
Thank you. The test looks reasonable (and passes with MF, but fails to compile with GC).
Would it be reasonable to have both this and the original test "partial replacement with a space" in?
I guess you've already checked that this works fine also when either the REPLACE or the copy content has a different kind of / amount of separators in - do you mind adding either another test case or adjust the new multiple one to test for that with and without LEADING?
I've glanced over the patch, too - also looks reasonable and the split of the replace code into a different function is also a good thing to do.
So thanks again.
Apart from the questions on the testsuite above this patch does reach the "critical mass" of being relevant for Copyright - Do you mind if I send you assignment papers to your mail address?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Status: this got somehow lost, I want to check the current state / inclusion in 3.2 for the release candidate after pushing my current changes (FUNCTION RANDOM via GMP, performance for comparisions and calculations and just got in: elementary with VALUES instead of VALUE (should be blocked, currently gets in but is not generated).
So this may take some days, but I'll definitely come back to this, planned not later than next week.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Thank you, if those are failing and you see a quick fix: feel free to commit it, too; otherwise check in with AT_XFAIL_IF([true]) so someone that cares can solve that.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Thank you very much for this well formed and complete patch!
Looks good. As I'm not sure about the initial idea of the removed code I've checked
svn blame, which showed the code you've removed for the fix was already in OC 1.0.So I've searched the history repo, found that Roger adjusted the style in [history:r2498] https://sourceforge.net/p/gnucobol/history/2498/ but the original change actually was done by Keisuke in [history:r2217] https://sourceforge.net/p/gnucobol/history/2217/ "improved text manipulation".
I still have no clue what this part of this commit done in 2004 is intended to do... @knishida Do you have an idea?
From "looking" at it it possibly is intended to strip away the line breaks and newlines from the queue (but it doesn't, it just checks the first byte, in which case it may would be possible to not add it to the queue in the first case).
@sbelondr please don't take this as offending, it is just a "core" place in our replacing code that obviously "worked" for all cases but this one since nearly 18 years... It is also undocumented and .. special.
I hope @knishida may have a look at this one. Nonetheless I'll very likely include this change end of next week (to be part of 3.2rc1).
Last edit: Simon Sobisch 2021-12-16
Hi @sf-mensch, do you have any information when you are going to push this patch ?
Will be pushed tonight or torrow. Thanks again for the patch and thanks for the ping, too.
Jut to recheck (will commit over the weekend): do you have a specific "target" for now (trunk or 3.x)?
Sorry for the late response. It's actually for 3.x.
I admit I don't know the difference between the trunk and the 3.x branch
Hi @sf-mensch,
A team encounters some problem with this patch. I will perform other tests to check if it comes from this patch.
I would come back here later.
Thanks - if it is then this case should be added to the test case - because we did not see anything broken before.
Hi,
I publish new patch because the last fix could cause a new error because it didn't check the other values to be replaced. I've provided a code to test.
I think that the while loop I removed matched this line of the MF (chapter REPLACE Statement, rule 2) :
For purposes of matching, each occurrence of a separator comma, semicolon or space in pseudo-text-1 or in the source program text is considered to be a single space. Each sequence of one or more space separators is considered to be a single space.For the new patch :
The problem with the replacing algo (ppecho function) is that it doesn't check the values correctly if there was a Partial match. For example :
copy.inc: 02 TEST-VAR PIC X(2) VALUE "OK". 02 TEST-OK PIC X(6) VALUE "OK BIS". main.cob: COPY "copy.inc" REPLACING LEADING ==TEST-VAR== BY ==TEST-AVR== == 02 TEST-OK == BY == 02 TEST-KO ==.When we have that, cobc handles it like this:
For this line:
02 TEST-VAR PIC X(2) VALUE "OK".:Search with
02-> no match forTEST-VAR, partial match for02 TEST-OKSearch with
02 TEST-VAR-> no match forTEST-VARand02 TEST-OKSearch with
PIC-> ...So during a Partial match, the function doesn't test on the next values (TEST-VAR) and goes to the next token (PIC).
I've added a loop to check all the words (queue) when it doesn't match after a Partial match.
Thank you. The test looks reasonable (and passes with MF, but fails to compile with GC).
Would it be reasonable to have both this and the original test "partial replacement with a space" in?
I guess you've already checked that this works fine also when either the REPLACE or the copy content has a different kind of / amount of separators in - do you mind adding either another test case or adjust the new multiple one to test for that with and without LEADING?
I've glanced over the patch, too - also looks reasonable and the split of the replace code into a different function is also a good thing to do.
So thanks again.
Apart from the questions on the testsuite above this patch does reach the "critical mass" of being relevant for Copyright - Do you mind if I send you assignment papers to your mail address?
Ok, I'm realizing other test. In my side, the test passed.
Yes you can send me assignment papers : samuel.belondrade@atos.net
You can find other tests case in this patch.
Status: this got somehow lost, I want to check the current state / inclusion in 3.2 for the release candidate after pushing my current changes (FUNCTION RANDOM via GMP, performance for comparisions and calculations and just got in: elementary with
VALUESinstead ofVALUE(should be blocked, currently gets in but is not generated).So this may take some days, but I'll definitely come back to this, planned not later than next week.
[r4852] now includes the patches (after minor adjustments, and swapping of
ppechoandppecho_replaceto minimize diff size).Related
Commit: [r4852]
Do we need a test case and potential follow-up fix for the same scenario in the listing code (cobc.c only)?
Yes I did not add tests in the listings part.
Last edit: Nicolas Berthier 2022-12-07
Actually I'll add those tests ASAP.
Thank you, if those are failing and you see a quick fix: feel free to commit it, too; otherwise check in with
AT_XFAIL_IF([true])so someone that cares can solve that.