CI for forward-porting GC3 patches to GC4 #147
Conversation
ddeclerck
force-pushed
the
gc3_to_gc4
branch
3 times, most recently
from
6900d8d
to
1c95bd0
Compare
build_aux/bootstrap
Outdated
| @@ -96,12 +96,15 @@ autoreconf $AC_OPTS $MAINPATH > $msgs 2>&1; ret=$? | |||
| # Filter aminclude_static as those are only used _within_ another | |||
| # check so reporting as portability problem is only noise. | |||
| # This has the effect of redirecting some error messages to stdout. | |||
| # to be moved to the Makefile - currently only usable for bootstrap, | |||
| # but should be done on autogen, too | |||
libcob/fileio.c
Outdated
| snprintf (file_open_env, (size_t)COB_FILE_MAX, "%s%s", "IO_", s); | ||
| if ((file_open_io_env = cob_get_env (file_open_env, NULL)) == NULL) { | ||
| snprintf (file_open_env, (size_t)COB_FILE_MAX, "%s%s", "io_", s); | ||
| if (f != NULL) { |
There was a problem hiding this comment.
When/why should f be null here?
There was a problem hiding this comment.
Because some functions (open_cbl_file, cob_sys_delete_file, ...) were "improved" to perform file mapping in GC3 (rev 3944), by calling the cob_chk_file_mapping function, which does not take a cob_file argument in GC3 but does in GC4, and that function in turn calls cob_chk_file_env. Since these functions (open_cbl_file, cob_sys_delete_file, ...) do not use a cob_file object, I resorted to passing NULL and coping with that...
There was a problem hiding this comment.
Is this okay for you @GitMensch ?
|
oops, hope I haven't broken the gitignore in f36dcda - if not then we likely should apply that to the gcos3x branch as well. |
Saw your message a bit late, added another commit in the meantime 😅 By wrapping up you mean, committing to SVN ? (after doing the requested modifications of course) |
|
This approach is reasonable in general.
... So you did already check that this piece of code changed in a later commit?
If you see a difference in the current code, then you can use "svn annotate" to check which commit did the change in this party of fileio.c to know what to merge before the refactoring.
:-)
Am 1. Juni 2024 00:07:13 MESZ schrieb OCP David Declerck ***@***.***>:
…
@ddeclerck commented on this pull request.
> + /* apply COB_FILE_PATH if set (similar to ACUCOBOL's FILE-PREFIX) */
+ if (file_paths) {
+ for(k=0; file_paths[k] != NULL; k++) {
+ snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s",
+ file_paths[k], SLASH_CHAR, file_open_name);
+ file_open_buff[COB_FILE_MAX] = 0;
+ if (access (file_open_buff, F_OK) == 0) {
+ break;
+ }
+#if defined(WITH_CISAM) || defined(WITH_DISAM) || defined(WITH_VBISAM) || defined(WITH_VISAM)
+ /* ISAM may append '.dat' to file name */
+ snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s.dat",
+ file_paths[k], SLASH_CHAR, file_open_name);
+ file_open_buff[COB_FILE_MAX] = 0;
+ if (access (file_open_buff, F_OK) == 0) {
+ snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s",
+ file_paths[k], SLASH_CHAR, file_open_name);
+ file_open_buff[COB_FILE_MAX] = 0;
+ break;
+ }
+#endif
+ }
+ if (file_paths[k] == NULL) {
+ snprintf (file_open_buff, (size_t)COB_FILE_MAX, "%s%c%s",
+ file_paths[0], SLASH_CHAR, file_open_name);
+ file_open_buff[COB_FILE_MAX] = 0;
+ }
+ strncpy (file_open_name, file_open_buff, (size_t)COB_FILE_MAX);
+ }
I remember why I haven't refactored that straight away : in case subsequent commits modify the same code, conflicts will be much easier to handle. I was thinking about keeping the refactoring for after all the patches are merged...
--
Reply to this email directly or view it on GitHub:
#147 (comment)
You are receiving this because you commented.
Message ID: ***@***.***>
|
|
I tend to be overly "conservative". Indeed this piece of code is barely modified afterwards, so I'll do the refactoring. |
|
Is this okay to merge (@GitMensch) ? |
|
I'll try to review this (late) evening. |
|
looks_absolute should use "src", not file_open_name directly (merge issue?) "apply_file_paths" should get that via argument as well and have a function comment that it writes to the global buffer. Then add a Changelog "extracted from xyz and also used in abc" to finish that last commit. We either have to remember for later that we need to add a testcase for the new use or (potentially easier) also include it in the last commit as well. |
This change is introduced in a later commit (3993).
Alright ; as for its output, should it write it through its argument or directly to
By "new use", de you mean the fact that we apply file paths to the complex case ? |
yes
good catch - then it is fine to leave as is; if you don't expect any big problem it would be nice to merge that in this bunch to commit that together, but a later bunch is fine as well
Depends on how other functions do it - it is best for now to mimic that (once the merge is completed we may revisit that part, but there are "some" commits left until we get there). |
|
I made the necessary changes.
I find it more convenient to merge consecutive commits. If that's okay for you I could add to the current batch the next eligible commits until 3993 (that would be 6 commits: 3973, 3979, 3988, 3989, 3992 and 3993). |
|
That batch is good to go :-) |
Merged in SVN ;) I see the next commits deal with translation files. Checking the history, it seems those files are usually just copied "as-is" from the GC3 branch to the trunk; is this correct ? |
No, only new files are copied, the others left as-is; before a release I regenerate the files but the files are nearly completely maintained by the translation project. And of course |
|
Alright.
Hope this unfinished sentence did not have any vital info 😅 |
|
I can't remember any important info missing there. |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## gc4 #147 +/- ##
======================================
Coverage ? 65.72%
======================================
Files ? 37
Lines ? 65593
Branches ? 18281
======================================
Hits ? 43113
Misses ? 15457
Partials ? 7023
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Quick question: I sometimes see alternative code for GC4 in I'm talking about those: |
|
That's quite a bunch - any reason to not merge upstream? [we really need to get to commits that have someone else in the ChangeLogs...] |
No good reason. It may be many commits, but the first batch had way more lines (this one is only +1,162 −904).
I'm looking forward to reaching commit 4614 - our first contribution to GC3 ;) |
|
I guess it's good enough for this batch (@GitMensch). |
I'm not sure what your hackish change results in, but it definitely looks wrong. /* Line: 19 : PERFORM : NC2.CBL */
module->module_stmt = 0x00100013;
module->statement = STMT_PERFORM;
cob_global_exception = -1;
b_9 = 1;
for (;;)
{
b_7 = 1;
/* Line: 20 : UNTIL : NC2.CBL */
module->module_stmt = 0x00100014;
module->statement = STMT_UNTIL;
if (((int)(b_9 - 10) == 0))
break;
for (;;)
{
b_5 = 1;
/* Line: 21 : UNTIL : NC2.CBL */
module->module_stmt = 0x00100015;
module->statement = STMT_UNTIL;
if (((int)(b_7 - 10) == 0))
break;
for (;;)
{
b_3 = 1;
/* Line: 22 : UNTIL : NC2.CBL */
module->module_stmt = 0x00100016;
module->statement = STMT_UNTIL;
if (((int)(b_5 - 10) == 0))
break;
for (;;)
{
/* Line: 23 : UNTIL : NC2.CBL */
module->module_stmt = 0x00100017;
module->statement = STMT_UNTIL;
if (((int)(b_3 - 10) == 0))
break;
cob_check_subscript (b_9, 10, "IDX-4", 0);
cob_check_subscript (b_7, 10, "IDX-3", 0);
cob_check_subscript (b_5, 10, "IDX-2", 0);
cob_check_subscript (b_3, 10, "IDX-1", 0);--> so only generate in the most inner one. Maybe "calculate" the most inner one first, then checking before the loop if we're already there and if yes start not from the current but the first one? |
Can you please check if that change works "as expected" in 3.x - and in this case commit it there and merge here as part of this bunch? |
Admittedly +4000/-2000 lines is quite big (didn't really realize as most of these lines were merged automatically, though I do read the resulting code to see if it makes sense). Maybe I should limit to +2000 lines per batch ? |
Whatever you find reasonable. Sometimes lines will only be formatting or similar, those are very easy to grasp in the diff review, sometimes even 200 lines be much, especially if you did need to do a bunch of changes to get it working as expected (=a plain merge is not working). |
Do you review each commit individually or do you review the whole batch ? |
|
mostly the batch, sometimes specific commits |
Actually And there are also a few occurrences of calls to |
It avoids checking the inner indices before their initial value is set (which resulted in bugs). But that was just a quick hack so I could keep working.
So we'll check all the indices at every iteration of the innermost loop - is that alright ? (I guess it is since one can arbitrarly tweak the values of indices from the PERFORMed code) |
|
That's absolute correct, those can even be overwritten by out-of-bound writes to different items. |
|
I'd like to say this is fixed, but... In NIST's SQ214A, we have: And then: So |
|
I'm confused - that is an index so and as long as it isn't used as subscript or Per standard that would be raised for its actual use in a subscript with an invalid value or if an arithmetic expression in If it would be out of the limits of its matching Checking further: COBOL2002 (which added exceptions) added to the As our implementor range is an So: it does seem that the whole generated check is wrong - that should only be done when the index is used within a subscript. Please check if that's already bad in GC 3.3 and adjust your fix for GC4. |
So, does that mean this bound check added (only) to 4.x by Ron in 4953 should in fact be removed ? Other than this, there is already a check both in 3.x and 4.x when the index is actually used. |
I see now where the issue is... The idea of that commit:
So the things that need to be done in a separate commit, ideally up-front upstream:
As that new flag will only be used in its specific test case (maybe compile and run w/wo) it won't break NIST any more, but provides a possible less-expensive way to check use of index-variables in production. What do you think? Note: "the future"TM should bring an adjusted codegen that has the subscript check for 101 MOVE TAB-F1(IDX) TO VAR
102 MOVE TAB-F1(IDX) TO TAB-F1(IDX+1), TAB-F2(IDX+1)
103 PERFORM SOMETHING.
104 IF TAB-F2 (IDX) NOT = VAR2
105 MOVE TAB-F2(IDX) TO VAR2, VAR3.
106 SET IDX UP BY 1
107 MOVE TAB-F1(IDX) TO TAB-F3(IDX) here the runtime check, which currently happens in all those cases) would be
Even then the new |
That seems reasonable. Should this be done in 3.x too, or just in 4.x ? |
|
I'm always happy if someone merges from 4 to 3, then do fixes there and merge it back to 4 :-) |
I guess I'll go for it then ;) |
|
Actually just realizing... Doesn't that interfere with 3.x's |
|
Possibly - can you please elaborate? Note: neither the warning part nor the extra |
I made a PR to discuss this: #191. |
|
The alternative is to merge the changes and move the new non-iso code into a But if you do that then please make that (the disabling + TODO entry + moving the syntax check to syn_occurs.at and make it an XFAIL if you can't keep the warning) a separate commit before the merge ones titled something like "disable r4953 index-check optimization to allow merging (should be enabled later as an optional flag)" or similar. |
You mean a direct commit into SVN trunk ? As for "moving the syntax check to syn_occurs.at", the new test introduced by 4953 in run_subscripts.at both has a compile-time check and a runtime check. Or were you referring to some other test ? |
yes, your merge here would then be based on that
That would mean (either with the disabling option or with the integration option to GC3) to change
|
Follow-up of #146.