This patch fixes a bug in COPY REPLACING, about spaces surrounding the replacement pseudo-text - #309
Conversation
|
Sounds good, it just fails all CI because it contains debug output (as noted before we have COB_DEBUG_LOG macros that would be ideally used instead) and SIGSEGVS. I'll check again once CI passes - or you drop a review-request :-) |
917db7a to
6306453
Compare
|
The Windows CI again, I think it's good otherwise after fixing some other tests. |
6306453 to
651c0d7
Compare
GitMensch
left a comment
There was a problem hiding this comment.
Thanks for bringing this up.
How did you got to the conclusion of a bug here? Possibly a rule in the standard, I guess.
Does the test code work on IBM/MF/ACU/gcobol?
And: is REPLACE affected as well?
Possibly the listing?
Some customer code triggered it (mainly because it would introduce a space, i.e.
I could only test it on MF, which is consistent with the fact that our customer's code was coming from MF. |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## gitside-gnucobol-3.x #309 +/- ##
=======================================================
Coverage ? 67.48%
=======================================================
Files ? 34
Lines ? 61566
Branches ? 16037
=======================================================
Hits ? 41547
Misses ? 14067
Partials ? 5952 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
c6c91ed to
14f5cbb
Compare
|
If the only problem now is the listing, I think we should merge this patch in SVN, keeping only an issue that the listing is broken (in general) with replacements. |
GitMensch
left a comment
There was a problem hiding this comment.
I agree that we can put that upstream once we added the listing issue to the known problems (it has its own bug already) and reference that in a comment (references like that also help a lot when fixing things and see bunch of failures and inspecting them directly brings up a note "currently wrong, should be this way, see x").
If possible it would be good to check if those issues are fixed now as well:
- crash when replacing by nothing (regression from your PR) https://sourceforge.net/p/gnucobol/bugs/1071/ - the analysis and fix is done, the commit including a testcase is likely missing
- REPLACE statement inside COPY does not work correct https://sourceforge.net/p/gnucobol/bugs/722/ - should be solved with your changes in 3.2
- COPY REPLACING isn't working when replacing with a star https://sourceforge.net/p/gnucobol/bugs/736/ - edge case possibly solved in 3.2
- COPY/REPLACE issues https://sourceforge.net/p/gnucobol/bugs/927/ - could be already solved in 3.x since a while
All that are not fixed yet can still be included in the testsuite as expected failures with the bug reference... but possibly they are solved / tested already or it is "relative easy" to do so.
|
side note: I'd prefer @ddeclerck's #303 to be upstreamed before, but if that isn't the case, then it is how it is |
Which part of #303 ? I have merged his changes to build_windows/ and libcob/ into the SVN, and created #310 for changes specific to GIT. |
|
I consider the "303 before" part done :-) |
1b62759 to
869c5e1
Compare
replacement pseudo-text should be ignored in the non LEADING/TRAILING case.
869c5e1 to
559e586
Compare
Fixed in this PR.
I think we can close all of them, the first one is similar to 831, and the other ones are, for me, not bugs. |
GitMensch
left a comment
There was a problem hiding this comment.
... yay, I've seem to have missed clicking "Submit" some hours ago, so here we go, mostly with a request on the testing
| STOP RUN. | ||
| ]) | ||
| AT_DATA([fab.cob], [>>SOURCE IS FREE | ||
| >>DEFINE 1aa AS 2 |
There was a problem hiding this comment.
I guess you wanted to check if replace also worked in the directive - I don't know if they should - but that isn't tested yet, to do so you need to do anything with the define also in the program (with the changed/unchanged name)
There was a problem hiding this comment.
Actually, this file comes from the issue on the bug tracker, I just took the files that were submitted and created a test case with them (only fob.cob was created because it was not in the issue)
| PROGRAM-ID. HELLO. | ||
|
|
||
| DATA DIVISION. | ||
| WORKING-STORAGE SECTION. |
There was a problem hiding this comment.
Shouldn't that test (and therefore also the keywords) include REPLACE as well?
There was a problem hiding this comment.
I haven't tried to create a perfect test case, just the one that was in the issue...
|
If it's ok with you, I am going to merge this PR as is, to prevent it from stalling. |
GitMensch
left a comment
There was a problem hiding this comment.
OK to send upstream, while I'd prefer this testcase to be reinspected - we can set the upstream issue(s) to pending for that while leaving a note that the bug itself is fixed with that commit.
Co-authored-by: Simon Sobisch <simonsobisch@web.de>
54cfa57 to
e8cb017
Compare
|
I updated the test to check also REPLACE and remove useless directives. |
|
Merged in SVN |
This patch fixes a bug in COPY REPLACING, when spaces are surrounding the replacement pseudo-text, and should be ignored in the non LEADING/TRAILING case.