Skip to content

Fix #1544: Allow long signatures in names #1588

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
Oct 16, 2016

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Oct 13, 2016

Fixes #1544 by making the length field use 1 or 2 bytes,
depending on the number of parameters in a signature.

Review by @Blaisorblade ?

Fixes scala#1544 by making the length field use 1 or 2 bytes,
depending on the number of parameters in a signature.
@odersky odersky changed the title Fix #1502: Allow long signatures in names Fix #1544: Allow long signatures in names Oct 13, 2016
Copy link
Contributor

@Blaisorblade Blaisorblade left a comment

Choose a reason for hiding this comment

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

Apart from the assertion this looks reasonable. (I can't reasonably review NameBuffer though).

op
val length = currentAddr.index - lengthAddr.index - 1
assert(length < 128)
putNat(lengthAddr, length, 1)
putNat(lengthAddr, length, lengthWidth)
Copy link
Contributor

Choose a reason for hiding this comment

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

My only question is: what happens when the (altered) assertion fails? There can't be bounds-checking, so assert(length < (1 << (7 * lengthWidth))) would be good. I assume you're sure this will never happen, and that you're currently right, that's why I propose an assert, which would trigger if NameBuffer gets out-of-sync and lengthWidth is 1 when it shouldn't. Or simply if somebody forgets to specify lengthWidth 😉.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's already an assertion in NameBuffer that we don't overflow, and which even comes with a comprehensible error message. So we are good.

Copy link
Contributor

@Blaisorblade Blaisorblade left a comment

Choose a reason for hiding this comment

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

Ah, the assertion in TastyBuffer.putNat—you're right, thanks!

@odersky odersky merged commit 009398b into scala:master Oct 16, 2016
@allanrenucci allanrenucci deleted the fix-#1502 branch December 14, 2017 16:59
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