Skip to content

Conversation

@Earlopain
Copy link
Collaborator

This makes it hard to do version checks against this value. The current version checks work because there are so few possible values, and they are exclusively !=/==` checks.

As an example, #3337 introduces new syntax for ruby 3.5 and uses PM_OPTIONS_VERSION_LATEST as its version guard. Because what is considered the latest changes every year, it must later be changed to parser->version == parser->version == PM_OPTIONS_VERSION_CRUBY_3_5 || parser->version == PM_OPTIONS_VERSION_LATEST, with one extra version each year.

With this change, the PR can instead write parser->version >= PM_OPTIONS_VERSION_CRUBY_3_5 which is self-explanatory and works for future versions.

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Looks great to me, I have noticed this as well as being rather brittle but didn't get around fixing it myself, thanks!

V3_4(2);
V3_4(2),
V3_5(3),
LATEST(V3_5.value);
Copy link
Member

Choose a reason for hiding this comment

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

Could be:

Suggested change
LATEST(V3_5.value);
LATEST(0);

So that there is no risk to forget updating it.
And then 0 would be translated when "parsing" the options (already done in pm_parser_init but not sure if that gets called when using the serialization).
Same for javascript/src/parsePrism.js below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that it is nicer that way. It works because pm_options_read (which does the deserialization is always followed by pm_parser_init. I only tested ffi though but they should all go through the same thing in the end.

@Earlopain Earlopain force-pushed the untangle-version-support branch from 6e16142 to 9924f87 Compare July 22, 2025 11:16
@eregon
Copy link
Member

eregon commented Jul 22, 2025

This looks ready to merge for me, except maybe the PR title could be clearer like:

Use comparison operators for parse versions checks instead of ==/!=
or
Make parse version values monotonic to clarify version checks

@tenderlove or @Morriar Could you do a second review and merge?

This makes it hard to do version checks against this value. The current version checks work because there are so few possible values at the moment.

As an example, PR 3337 introduces new syntax for ruby 3.5 and uses `PM_OPTIONS_VERSION_LATEST` as its version guard. Because what is considered the latest changes every year, it must later be changed to `parser->version == parser->version == PM_OPTIONS_VERSION_CRUBY_3_5 || parser->version == PM_OPTIONS_VERSION_LATEST`, with one extra version each year.

With this change, the PR can instead write `parser->version >= PM_OPTIONS_VERSION_CRUBY_3_5` which is self-explanatory
and works for future versions.
@Earlopain Earlopain force-pushed the untangle-version-support branch from 9924f87 to 8318a11 Compare July 23, 2025 08:44
@tenderlove tenderlove merged commit 9f13342 into ruby:main Jul 29, 2025
58 checks passed
@Earlopain Earlopain deleted the untangle-version-support branch October 3, 2025 10:37
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.

3 participants