Add optional index check #191
Conversation
| if (CB_FIELD_PTR (sub)->flag_indexed_by) { | ||
| continue; | ||
| } | ||
| } | ||
| if (cb_subscript_check != CB_SUB_CHECK_MAX |
There was a problem hiding this comment.
That's where I'm wondering if the added lines above interact badly with this.
There was a problem hiding this comment.
No need to wonder (my guess is this breaks cb_subscript_check: max) - but a reason to add this into the "enable / disable subscript check with ODO" test (run_subscripts.at) tests, which should get a prog2.cob in any case that uses an index in any case (which should behave identical for the current tests but likely break with the new option [which only makes sense in the prog2.cob test as it only applies to indexes).
... while you at this: please replace the leading tabs in that test's prog.cob.
| @@ -707,7 +707,9 @@ AT_DATA([prog.cob], [ | |||
| STOP RUN. | |||
| ]) | |||
|
|
|||
| AT_CHECK([$COMPILE prog.cob], [0], [], []) | |||
| AT_CHECK([$COMPILE -fopt-check-subscript-set prog.cob], [0], [], | |||
There was a problem hiding this comment.
Had to explicitly activate the flag here - but is it what we want ?
There was a problem hiding this comment.
Does the result differ otherwise?
As we found that this is a non-standard optimization it should only be enabled by explicit request.
There was a problem hiding this comment.
Actually 4935 added the warning to this test - but to get the warning we have to add the new -fopt-check-subscript-set flag.
Maybe we can test both behaviors: without the flag and the warning (as it was before this patch), and with the flag and the warning ?
There was a problem hiding this comment.
No, one of the point of the back-merge was that the checks and therefore the warnings should always be done, only the codegen depend on the new flag.
... but the warnings belong into a test in syn_occurs.at, not into a runtime check (please move it). And yes, the other runtime test should be done for both cases - w/wo the new option (without is missing and the compile command for the current test uses definitely more options than needed ($COMPILE -fnew-option prog.cob should be enough, if not it is reasonable to do minor necessary adjustments to the testcase).
There was a problem hiding this comment.
This is a new cobc option and should have an entry in the NEWS.
Your guess about the broken combination is likely correct and you'd need to adjust the new code to also include the new dialect option added in the meantime - but the (currently missing) extended test case will show if this is the case.
Note: please reword the help in flag.def, maybe something like "for index-names, don't apply EC-BOUND-SUBSCRIPT on subscript use per ISO rules but but on adjusting the index-name via SET or PERFORM statements, to reduce the amount of checks done".
I also wonder if we should place "index" into that option somewhere...
Please include into cobc/Changelog both Ron's original entry (with a remark "merged on YYYY-MM-DD as new option) and your on-top changes (the option and possibly inclusion into the dialect code) with that date.
| if (CB_FIELD_PTR (sub)->flag_indexed_by) { | ||
| continue; | ||
| } | ||
| } | ||
| if (cb_subscript_check != CB_SUB_CHECK_MAX |
There was a problem hiding this comment.
No need to wonder (my guess is this breaks cb_subscript_check: max) - but a reason to add this into the "enable / disable subscript check with ODO" test (run_subscripts.at) tests, which should get a prog2.cob in any case that uses an index in any case (which should behave identical for the current tests but likely break with the new option [which only makes sense in the prog2.cob test as it only applies to indexes).
... while you at this: please replace the leading tabs in that test's prog.cob.
|
Actually I'm not very confident I could do that quickly enough. I'd rather delegate this so I could focus on the GC3/GC4 merge - the clock's ticking and the days allocated to this task are almost depleted. |
|
I was surprised you took on that backport. The issue that I've now realized is - if you want to merge the dialect option from GC3 to GC4 then this has to be done in any case :-/ |
True indeed ; that dialect option is commit 5087 (so this will be a problem in ~50 commits). Though I'm just realizing now that feature was implemented by Nicolas ; I'm gonna ask him for help. |
This PR attemps to backport SVN commit 4953 to 3.x.