Skip to content
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

Add C#-style type accelerators and suffixes for ushort, uint, ulong, and short literals #7813

Merged
merged 12 commits into from Sep 28, 2018

Conversation

Projects
None yet
5 participants
@vexx32
Copy link
Contributor

commented Sep 18, 2018

PR Summary

This has been split off of #7509 at request of @iSazonov as that PR has become overly complex, and this portion of the PR was already approved by Powershell Committee. Re-submitting instead of working with #7575 due to messy rebase shenanigans that just... made a mess.

This fixes #3313.

Brief summary:

Adds the following numeric literal suffixes:

  • 'u' suffix (uint/ulong)
  • 's' suffix (short)
  • 'ul' suffix (ulong)
  • 'us' suffix (ushort)

Adds the following type accelerators:

  • [short]
  • [ushort]
  • [uint]
  • [ulong]

And of course, adds tests for all the above.

Comprehensive Summary:

  • Implements type suffixes s (short) and u (unsigned) to complement the existing l suffix.
    • Suffixes can be combined as us or ul to result in ushort or ulong types.
  • When u suffix is used alone, uint will be returned unless value exceeds uint.MaxValue, in which case it will automatically resolve as ulong instead.
  • Adds new type accelerators to fit new numeric types better into the PS ecosystem:
    • [short] => Int16
    • [ushort] => UInt16
    • [uint] => UInt32
    • [ulong] => UInt64
  • Maintains parity with existing l / L suffix, where it will also perform rounding functions when applied to a real literal while also ensuring it still returns an appropriate uint or ulong value: 105.5u => 106
  • Defines a new [Flags] enum NumberSuffixFlags that takes care of one-or-more numerical type suffixes.
  • Use of u, l, s and valid combinations thereof will error out and register as an invalid numeric literal if their value after multiplication exceeds the type(s) contraints.
    • This is intentional; if the user specifies a value that cannot be satisfied, such as 500sgb (500gb -as Int16)then the parser should not be expected to figure out what they meant.
  • Changes strNum parameter of TryGetNumberValue to ReadOnlySpan<char> to reduce the number of string allocations required for parsing.

PR Checklist

vexx32 added some commits Sep 18, 2018

Add tests for parsing new numeric types
Update numeric parsing tests
@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2018

@iSazonov I think this is tidier now. shudders I need to take a rebasing class! 😝

The remaining build errors appear to be build environment errors unrelated to this PR. ^^

The CodeFactor issues are (I think) not particularly serious, and I have a follow-up PR that will address the vast majority of those coming shortly.

@iSazonov iSazonov requested a review from SteveL-MSFT Sep 19, 2018

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2018

@vexx32 To rebase you can use the pattern:

# rebase pull request on top of master
git chechout <branch>
git fetch PowerShell master
git pull --rebase PowerShell master
git push -f

You can catch a conflict but it is another story.

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2018

Reopened to restart CIs. Looks all good, I guess the Linux issues were resolved.

@iSazonov I have never had the good fortune to get a rebase to work without painstaking editing at every step along the way. Not sure why... I think I must be doing it wrong somehow. I need to read up on it! 😄

@daxian-dbw daxian-dbw self-assigned this Sep 20, 2018

@SteveL-MSFT
Copy link
Member

left a comment

LGTM

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Sep 26, 2018

@daxian-dbw Could you please look th PR? @vexx32 want to continue working on the code in follow PR.

@daxian-dbw
Copy link
Member

left a comment

About the comments regarding the tests, I think it makes sense to keep some of the existing tests. They can be put only in the #Standard numeric notation section, not in other sections for the new notations.

@daxian-dbw

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

Marked this PR with Breaking-Change label. The new type suffix makes some command names be parsed as numbers now, for example:

function 100u { "Yay" }
100u
@daxian-dbw

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

@vexx32 I'm trying to tidy the PR description because the documentation work that follows up will need the accurate information.

'u' suffix (uint32)

By looking into the code, the suffix u doesn't guarantee uint32. It tries to fit the literal value into uin32 first, but if it fails, ulong will be used. Is taht implemetnation intentional? If so, please update the description.

I copied the Comprehensive Summary from #7509 as it's more relevant to this PR. But the summary is a little outdated compared with the implementation. Can you please update the summary? Thanks!

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

@daxian-dbw Yes, the automatic step-up to ulong is intentional. I felt that because u can be read simply as "unsigned" it makes the most sense to apply it in keeping with the existing patterns as best it can (i.e., a large integer currently will parse as long; thus, a large unsigned integer converts up to ulong when it exceeds uint32 bounds). I'll update the description and try to make sure everything fits with how it's intended to be. 😄

vexx32 added some commits Sep 28, 2018

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

@daxian-dbw I think I got them all, but let me know if I missed anything! 😄

PR description has been cleaned up and made hopefully useful as a jumping off point for some proper documentation.

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

Aww, I was enjoying the all-green. Appveyor's failing on a Test-Connection due to timeout.

@daxian-dbw
Copy link
Member

left a comment

LGTM. Thanks @vexx32 for your contribution! I will restart the AppVeyor CI.

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

Yay! 😄

Now I can get the next one ready for Hacktoberfest 😉

AppVeyor's still having difficulty with Test-Connection, it seems...

@daxian-dbw daxian-dbw merged commit 8f4b66a into PowerShell:master Sep 28, 2018

8 checks passed

CodeFactor 25 issues fixed. 18 issues found.
Details
PowerShell-CI-linux #PR-7813-20180928.05 succeeded
Details
PowerShell-CI-macos #PR-7813-20180928.05 succeeded
Details
PowerShell-CI-spelling #PR-7813-20180928.05 succeeded
Details
PowerShell-CI-windows #PR-7813-20180928.05 succeeded
Details
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details

@vexx32 vexx32 deleted the vexx32:Tokenizer/NumericSuffixes branch Sep 28, 2018

@vexx32 vexx32 referenced this pull request Oct 1, 2018

Closed

Numeric Literal Suffixes documentation needs to be expanded #2965

2 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.