Skip to content

Conversation

@emilienlemaire
Copy link

@emilienlemaire emilienlemaire commented Dec 18, 2024

This PR merges all the changes from svn 2926 and a few lines from 3064.

Based on the discussion started with #147 (comment)

When a COMP-X value is only one byte long, we always display a 3 character long character (as one byte can display integer up to 255)

Copy link
Collaborator

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do wonder on the need of the compx_size attribute and the field's alphanumeric category...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of those changes are either unnecessary or at the wrong place - CB_USAGE_COMP_X is never to be of CB_CATEGORY_ALPHANUMERIC (only it's ->pic could) but has always to be CB_CATEGORY_NUMERIC.

int size; /* Field size */
int level; /* Level number */
int memory_size; /* Memory size */
int compx_size; /* Original COMP-X byte size */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we just use f->pic->size for this rarely used definition, instead of adding an extra field to all variables?

@emilienlemaire
Copy link
Author

Hi Simon,
as mentioned in the description comment, this PR is mainly based on changes from SVN for GC4.
As such I took all changes from one of the commits (which include the addition of the compx_size field),
and a (very) small part from another one. I choose to take the whole part from the first so those changes
won't need to be made again when going to GC4.
I can take less from the mentioned commit, if needed.

@GitMensch
Copy link
Collaborator

Thanks Emilien,

I do understand the approach, but question if the solution in the rw-branch (which was later merged to pangea which later became the new trunk = GC4) was the correct thing to do in the first place.

In any case - please inspect the real commit here - r614.
This also allows you to merge the original changelog entry (extending that would be fine) with a possible new Changelog entry about your adjustments, referencing the date of that original entry.

Question: is there a reason to not use the testcase from the original commit instead of the new one (=misses it something)?

@emilienlemaire
Copy link
Author

I had the tests before editing the code, so I used it to validate the changes and it seemed to me that the other test did more than just check for the display value. Thinking about it again the original test should be used so everything from it is tested again.

@emilienlemaire emilienlemaire changed the title COMP-X displayed with 3 characters when size is one byte COMP-X displayed with number of characters from maximum value Dec 20, 2024
@emilienlemaire
Copy link
Author

This is now taking only changes from r614, once again I think this is the way to go for an easier merge of later commits that uses what r614 changed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants