Open ddeclerck opened 5 months ago
Hi @GitMensch, Would you have a bit of time to review this PR ? This is a blocking issue for our client. Thanks
Doing so now (I had quite a lot on my desk [don't ask how it currently looks like ;-) - but as my evening is free I'm inspecting this now, then go on to the other non-reviewed ones.
I had quite a lot on my desk [don't ask how it currently looks like ;-)
Actually curious: how does it look like ? :D
Inspecting strings.c str_cob_low
-> that always uses 0x00
; if we put the low- and high-value into the cob_program
, then we could make str_cob_low.data
point to that.
Thoughts?
Inspecting strings.c
str_cob_low
-> that always uses0x00
; if we put the low- and high-value into thecob_program
, then we could makestr_cob_low.data
point to that.Thoughts?
Yeah, that's something I should do.
Inspecting strings.c
str_cob_low
-> that always uses0x00
; if we put the low- and high-value into thecob_program
, then we could makestr_cob_low.data
point to that. Thoughts?Yeah, that's something I should do.
Actually not sure how to do that.
I've moved low_value
and high_value
from typeck.c
to the cb_program
strucure in tree.h
.
codegen.c
uses that information to output the cob_all_low
and cob_all_high
fields to the local include files (before that PR, that was in the global include file).
Now I don't know what to do with str_cob_low
. Right now it is a static variable. I guess it would require making it non-static, and make it depend on the current program...
Or should I just put the cob_all_low
and cob_all_high
fields in cob_module
instead, so I can use them through COB_MODULE_PTR
in strings.c
?
Or should I just put the
cob_all_low
andcob_all_high
fields incob_module
instead, so I can use them throughCOB_MODULE_PTR
instrings.c
?
You could do that, using the old value if those aren't set. Note that you don't need the the fields referenced there, it would be enough to have a pointer to a string there containing its .data
(which would be a string constant in the generated code.
If the pointers are NULL you'd use the old values (always "\x00"
/ "\xFF"
, I guess).
Now I don't know what to do with
str_cob_low
. Right now it is a static variable. I guess it would require making it non-static, and make it depend on the current program...
For making it work it would be enough to set its .data
according to the current program. For making it good, you'd place that locally in one of the callers, (where you then also setting the .data
) and pass the reference around as/if needed.
Attention: Patch coverage is 88.63636%
with 5 lines
in your changes are missing coverage. Please review.
Project coverage is 65.91%. Comparing base (
5713357
) to head (60568a2
).
Files | Patch % | Lines |
---|---|---|
cobc/codegen.c | 90.00% | 2 Missing :warning: |
libcob/strings.c | 50.00% | 1 Missing and 1 partial :warning: |
cobc/typeck.c | 94.44% | 0 Missing and 1 partial :warning: |
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I think that's it ?
Just from reading the Changelog: we now have a low-value field in the cob_module
and cb_program
; shouldn't we have the collating sequence in there already (or if not: add it)? If so, couldn't we just use its first (or last) position?
Just from reading the Changelog: we now have a low-value field in the module; shouldn't we have the collating sequence in there already (or in general)?If so, couldn't we just use its first position?
True indeed, I oversighted this.
We could directly use module->collating_sequence[0]
- when collating_sequence
is not NULL
.
I'll make the change.
Thanks - before doing that, what about the compiler (see the edited comment above)?
Just from reading the Changelog: we now have a low-value field in the
cob_module
andcb_program
; shouldn't we have the collating sequence in there already (or if not: add it)? If so, couldn't we just use its first (or last) position?
Yes, cob_module
already contains the collating sequence (as an array of characters).
I made the change to use that in strings.c
.
As for cb_program
, the collating sequence field is a cb_tree
, and it is slightly more involved to determine the low and high values (see cb_validate_collating
in typeck.c
), so I believe it is a good idea to cache the result of this computation in the cb_program
fields low_value
and high_value
. What do you think ?
Just a note: using MF VC the tests all fail, no matter what you define or what you have in CHARSET directive or in the MFCODESET variable at runtime: low-value stays 1, high-value stays 256.
But it does always return the expected X < HIGH-VALUE
.
What do you think ?
I think you're right, having it one-time resolved (after the OBJECT-COMPUTER
paragraph was parsed) and stored to cb_program
makes sense.
Just a note: using MF VC the tests all fail, no matter what you define or what you have in CHARSET directive or in the MFCODESET variable at runtime: low-value stays 1, high-value stays 256.
Ah, this is interesting. So MF always use 1 and 256 for low and high value ? Should we add a dialect option to reproduce this behavior in GnuCOBOL ?
Something ... is going on there, I need to do more checks on MF (my latest test were confusing) but I need to do something else before going on with that.
Something ... is going on there, I need to do more checks on MF (my latest test were confusing) but I need to do something else before going on with that.
Hi @GitMensch, Do you have any news about this matter ?
Something ... is going on there, I need to do more checks on MF (my latest test were confusing) but I need to do something else before going on with that.
Hi @GitMensch , Did you have a chance to investigate this further ?
Got my hands on a MF demo. Interrestingly, using ORD
on LOW-VALUE
and HIGH-VALUE
always give the same result (1 and 256), no matter the encoding. However, exploring the internal representation of fields (using REDEFINES
with USAGE BINARY-CHAR
) after moving LOW-VALUE
and HIGH-VALUE
do show the expected values. Maybe it's just ORD behaving differently on MF ?
Hi @GitMensch , Is there anything we can do about that ? (we keep being asked about this patch)
This attempts to fix https://sourceforge.net/p/gnucobol/bugs/948/.
I the loading of collating tables has been moved from
codegen
totypeck
. On loading the program collating sequence, we compute the low and high values and store them in global variables. Codegen now use these variables instead of hard-coded 0/255/0xff. I've also checked the rest of the code to ensure I wasn't breaking anything (I paid attention to the distinction betweencb_low
/cb_high
andcb_norm_low
/cb_norm_high
).I also replaced the hard-coded
cob_refer_ascii
andcob_refer_ebcdic
tables by the loaded tables (since their content matches the one defined indefault.ttbl
).Added a few tests, in particular the one that was failing for our client.