Skip to content

Parse versions as three integer tokens #1440

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Mar 24, 2023

Previously, internals from the lexer leaked through by representing the major and minor part of the version as a float literal. Also, we accepted non-decimal numbers (eg. hex) for version number, which we don’t want.

Fixes #1434
rdar://107152155

@ahoppen ahoppen requested a review from bnbarham March 24, 2023 18:43
@ahoppen
Copy link
Member Author

ahoppen commented Mar 24, 2023

@swift-ci Please test

@ahoppen ahoppen force-pushed the ahoppen/version-parsing branch from abd7eeb to cbf27d5 Compare March 24, 2023 18:47
@ahoppen ahoppen force-pushed the ahoppen/version-parsing branch from cbf27d5 to 0a57b4e Compare March 24, 2023 18:51
@ahoppen
Copy link
Member Author

ahoppen commented Mar 24, 2023

@swift-ci Please test

)
)
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Worth having custom diagnostics for these? It would be nice if the non-digit-containing token was unexpected within the version tuple rather than ending up as unexpected within the attribute when there's no space after ..

Copy link
Member Author

Choose a reason for hiding this comment

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

My judgment was that the diagnostics were sufficiently good for now.

@ahoppen ahoppen force-pushed the ahoppen/version-parsing branch from 0a57b4e to 10ef365 Compare March 24, 2023 20:43
@ahoppen
Copy link
Member Author

ahoppen commented Mar 24, 2023

@swift-ci Please test

@grynspan grynspan self-requested a review March 25, 2023 12:40
@ahoppen
Copy link
Member Author

ahoppen commented Mar 27, 2023

@swift-ci Please test Windows

name: "MajorMinor",
kind: .token(choices: [.token(tokenKind: "IntegerLiteralToken"), .token(tokenKind: "FloatingLiteralToken")]),
description: "In case the version consists only of the major version, an integer literal that specifies the major version. In case the version consists of major and minor version number, a floating literal in which the decimal part is interpreted as the minor version."
name: "Major",
Copy link
Member

@rintaro rintaro Mar 27, 2023

Choose a reason for hiding this comment

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

Only "patch" has the name "PatchVersion" we should be consistent

Previously, internals from the lexer leaked through by representing the major and minor part of the version as a float literal. Also, we accepted non-decimal numbers (eg. hex) for version number, which we don’t want.

Fixes swiftlang#1434
rdar://107152155
@ahoppen ahoppen force-pushed the ahoppen/version-parsing branch from 10ef365 to 47a8dcd Compare March 27, 2023 18:22
@ahoppen
Copy link
Member Author

ahoppen commented Mar 27, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Mar 27, 2023

@swift-ci Please test Windows

@ahoppen ahoppen merged commit c7dd3f4 into swiftlang:main Mar 28, 2023
@ahoppen ahoppen deleted the ahoppen/version-parsing branch March 28, 2023 17:36
ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Mar 31, 2023
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.

VersionTupleSyntax should be parsed as (up to) 3 integer literals
4 participants