GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add C#-style type accelerators and suffixes for ushort, uint, ulong, and short literals. by vexx32 · Pull Request #7575 · PowerShell/PowerShell

Conversation

@vexx32

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) commented Aug 20, 2018

Copy link
Copy Markdown
Collaborator

PR Summary

This has been split off of #7509 at request of Ilya (@iSazonov) as that PR has become overly complex, and this portion of the PR was already approved by Powershell Committee.

Adds the following numeric literal suffixes:

  • 'u' suffix (uint32)
  • '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.

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • User-facing Documentation needed
      • Issue filed - Issue link:
  • Testing - New and feature
    • Not Applicable or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
      • Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Rain Sallow (/u/ta11ow) (vexx32) added 8 commits August 12, 2018 17:44
@vexx32
Merge pull request #1 from PowerShell/master
f3b0413 
Catch up to PowerShell/PowerShell/master
@vexx32
Merge pull request #2 from PowerShell/master
41e64d2 
Catch up to master
@vexx32
Merge branch 'master' of git://github.com/PowerShell/PowerShell
8d4e16c
@vexx32
Add typesuffix traits to u and s
a636dca
@vexx32
Add type accelerators for uint, ulong, ushort, and short
4330f28
@vexx32
Add support for parsing uint, ulong, ushort, and short values
03739e1
@vexx32
Add tests for parsing the new literals
377f7b2
@vexx32
Add tests for new type accelerators
6f84b5a
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) requested review from Bruce Payette (BrucePay) and Dongbo Wang (daxian-dbw) as code owners August 20, 2018 16:57
@vexx32
Overlooked a suffix reference I need to change the type of.
1730a45
@vexx32 Rain Sallow (/u/ta11ow) (vexx32) mentioned this pull request Aug 20, 2018
Refactor the tokenizer #7509
Closed
6 of 8 tasks
@vexx32
Remove weird copy paste error
74c9c1a 
How did that even happen, seriously...
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Aug 20, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

I don't intend to fix the code factor issues here. I'll address them during the parser refactor that will come after this. (Much of the code causing the majority of the issues will be replaced anyhow; see #7509).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov
Ilya (iSazonov) reviewed Aug 21, 2018
View reviewed changes
Comment thread src/System.Management.Automation/engine/parser/tokenizer.cs Outdated
else
{
result = d * multiplier;
result = d;

@iSazonov Ilya (iSazonov) Aug 21, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why we remove the multiplier?
What scenario address the code? It would nice to add a comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 21, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ah, I thought I got all those. Correcting...

(I was trying to see if I could do the multiplier before the conversions to avoid throwing too much here, but it doesn't work because Int64.MaxValue is too high to retain precision with Double values in the upper range there.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/parser/tokenizer.cs Outdated
result = ((long)Convert.ChangeType(d, typeof(long), CultureInfo.InvariantCulture)) * multiplier;
result = ((long)Convert.ChangeType(d, typeof(long), CultureInfo.InvariantCulture) * multiplier);
}
else if (suffix == NumberSuffixFlags.Short)

@iSazonov Ilya (iSazonov) Aug 21, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I believe we could replace one level of if-else-if with switch to get more readable code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 21, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Hmmm. Agreed!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/parser/tokenizer.cs Outdated
{
if (short.TryParse(strNum, style, NumberFormatInfo.InvariantInfo, out short s))
{
result = s * (short)multiplier;

@iSazonov Ilya (iSazonov) Aug 21, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

What result do we expect for multiplier = 1Mb?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 21, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

At present, that will throw an error (too big for short value type) and is expected.

With some of the upcoming changes (that you can see if you build the version in the original PR) that instead simply registers as an invalid numeric literal, as I am able to verify first that the multiplier does not overflow the intended value type bounds without introducing a morass of if/else.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 21, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Closed under next PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/parser/tokenizer.cs Outdated
}
else if (suffix.HasFlag(NumberSuffixFlags.Unsigned) && ulong.TryParse(strNum, style, NumberFormatInfo.InvariantInfo, out ulong u))
{
u *= (ulong)multiplier;

@iSazonov Ilya (iSazonov) Aug 21, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I wonder that we define multiplier as long - I expect ulong.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 21, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I am also pondering that myself, but I've not seen a completely necessary need to change it. It does seem to simplify calculations against other signed types, however; unsigned types often need a cast one way or the other when doing multiplication with signed types.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 21, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Language/Parser/Parser.Tests.ps1 Outdated
@{ Script = "0x100000000"; Expected = [int64]0x100000000 }
@{ Script = "0x100000000"; Expected = [int64]0x100000000 }
#Tests for short notation
@{ Script = "10s"; Expected = "10" }

@iSazonov Ilya (iSazonov) Aug 21, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think now we should test not only value but target type too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 21, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

That is probably wise. I'll take some time over lunch to add tests for value types. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Language/Parser/Parser.Tests.ps1

$testInvalidNumerals = @(
@{ Script = "16p" }
@{ Script = "20ux" }

@iSazonov Ilya (iSazonov) Aug 21, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Should we add 20x?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 21, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sure, if we want to. x is neither a valid type suffix nor a multiplier in PS, though, so testing for it is more or less the same as testing 16p at the moment. We could go through the alphabet if you want. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 21, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

No :-) We should cover all code paths. Do we cover?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 21, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

x should result in an error. I'll pop it in. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Language/Parser/Parser.Tests.ps1
@{ Script = "21ss" }
@{ Script = "100ll" }
@{ Script = "150su" }
@{ Script = "10ds" }

@iSazonov Ilya (iSazonov) Aug 21, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Should we add 10ud?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 21, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ah, good catch. Absolutely.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 21, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ilya (@iSazonov) Also noting down now before I forget:
Presently the literal 0x10d is legal in PowerShell, although Decimal.TryParse() does not allow the hexadecimal syntax and that parse fails. Its actual return type is Int32.

I plan on fixing this as I go through the parser rewrite after this PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 21, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

0x10d - have we tests for it?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 21, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Oh, lol. I am clearly not quite up to speed this morning. Of course 0x10d is legal. D is a hexadecimal character... and that's exactly why decimals will never result from a hex literal ending in d. I'm gonna make a comment in the code that reflects this, because it's not immediately obvious why Decimal doesn't even bother to check for hex literals.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 21, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

(and yes I believe there are tests for it, but I'll pop one in the type checks just to be sure.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 21, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Added all these. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/parser/tokenizer.cs Outdated
result = null;
return false;
}
else if (suffix == NumberSuffixFlags.Short)

@iSazonov Ilya (iSazonov) Aug 21, 2018

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I believe we could replace one level of if-else-if with switch to get more readable code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) Aug 21, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Again, agreed. Sorted out. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov Ilya (iSazonov) self-assigned this Aug 21, 2018
@iSazonov Ilya (iSazonov) added the Committee-Reviewed PS-Committee has reviewed this and made a decision label Aug 21, 2018
@iSazonov

Ilya (iSazonov) commented Aug 21, 2018

Copy link
Copy Markdown
Collaborator

PowerShell Committee approved this in #7509.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Ilya (iSazonov) requested review from Steve Lee (SteveL-MSFT) and Rob Holt (rjmholt) August 21, 2018 12:21
Rain Sallow (/u/ta11ow) (vexx32) added 6 commits August 21, 2018 08:45
@vexx32
Address review comments
dd4c6c2 
Partially replace if stack with switches
Add missing multiplier.
@vexx32
Add More Tests for new literal types.
6d73617
@vexx32
Add comments for decimal weirdness & some tests
c37e531
@vexx32
Add test for ud suffix that should fail
3b058d7
@vexx32
Fix short parsing
a347629 
Apparently int16 * int16 always results in int32.

Wat.
@vexx32
Forgot the decimal case. Fixed that short parsing.
c628341
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Aug 21, 2018

Copy link
Copy Markdown
Collaborator Author

OK, pretty sure I got that short parsing sorted. I can understand doing operations in the console types can be freely converted, but if you're gonna specify s and get an int32, there's not much of a point!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rjmholt Rob Holt (rjmholt) added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Aug 21, 2018
@vexx32
Build framework for accepting and recognising named arguments for met…
4ffe824 
…hods
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Sep 18, 2018

Copy link
Copy Markdown
Collaborator Author

Attempting... this always seems far messier than it should be.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Rain Sallow (/u/ta11ow) (vexx32) added 22 commits September 18, 2018 04:34
@vexx32
Add support for parsing uint, ulong, ushort, and short values
f59cc1e
@vexx32
Add tests for parsing the new literals
cf2e419
@vexx32
Remove weird copy paste error
01ad913 
How did that even happen, seriously...
@vexx32
Address review comments
8135b27 
Partially replace if stack with switches
Add missing multiplier.
@vexx32
Add More Tests for new literal types.
7417725
@vexx32
Add comments for decimal weirdness & some tests
49212ac
@vexx32
Add test for ud suffix that should fail
e677fbb
@vexx32
Fix short parsing
7c2722c 
Apparently int16 * int16 always results in int32.

Wat.
@vexx32
Add clarifying comment to uint parsing cases
583287a
@vexx32
Reorder parsing cases for unsigned ints
8acc402
@vexx32
Revise parser notation tests
4736c18
@vexx32
Fix typo in test
d8cb6be
@vexx32
Expand numeric parsing test cases
920faca
@vexx32
Add explicit multiplier value tests
fb1f216
@vexx32
Fix Tests' expected values.
581c0dc
@vexx32
Cover *all* the test cases!
b711eb4
@vexx32
Fix testcases reference
fec8ef2 
Teach me to try to restructure the test file too much! ;)
@vexx32
Fix typos in tests
bffa60d
@vexx32
Add test cases for numbers beginning with +
bb818d9 
Fix test cases for reals with certain suffixes
@vexx32
Revert changes to AST that crept in from another branch
5ceba6e 
No, really, I have no idea how the heck this happened.
It's gone now.
@vexx32
Manual revert ast.cs to original
cd2bc8e 
Because I cannot find a distinct commit that actually modified this file
@vexx32
Merge branch 'Tokenizer/TypeSuffixes' of https://github.com/vexx32/Po…
a6eb532 
…werShell into Tokenizer/TypeSuffixes
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Sep 18, 2018

Copy link
Copy Markdown
Collaborator Author

Okay... Ilya (@iSazonov) apologies about the mess. Not sure what it is with me and rebasing, but I've yet to manage to figure it out and get it done cleanly. The Linux build seems to be failing on some kind of permissions issue with that build environment, not sure what that's about.

Everything else looks green, though. :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Sep 18, 2018

Copy link
Copy Markdown
Collaborator

You should reset to your last commit and then rebase.
Or create new branch, copy your commits and open new PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vexx32 Rain Sallow (/u/ta11ow) (vexx32) mentioned this pull request Sep 18, 2018
Add C#-style type accelerators and suffixes for ushort, uint, ulong, and short literals #7813
Merged
6 of 10 tasks
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Sep 18, 2018

Copy link
Copy Markdown
Collaborator Author

Ilya (@iSazonov) yeah, point taken. Rebasing is something I've yet to get to grips with. CLosing this for now, let's continue from a cleaner slate in #7813, where everything's all ready. :D
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vexx32 Rain Sallow (/u/ta11ow) (vexx32) closed this Sep 18, 2018
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) deleted the Tokenizer/TypeSuffixes branch September 19, 2018 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov approved these changes
@BrucePay Bruce Payette (BrucePay) Awaiting requested review from BrucePay
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw daxian-dbw is a code owner
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
@rjmholt Rob Holt (rjmholt) Awaiting requested review from rjmholt

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@iSazonov Ilya (iSazonov)

Labels

Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@vexx32 @iSazonov @rjmholt @SteveL-MSFT
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.