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

vexx32 · 2018-08-20T16:57:45Z

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.

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

Catch up to PowerShell/PowerShell/master

Catch up to master

How did that even happen, seriously...

vexx32 · 2018-08-20T19:42:42Z

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).

iSazonov · 2018-08-21T11:48:44Z

src/System.Management.Automation/engine/parser/tokenizer.cs

                            else
                            {
-                                result = d * multiplier;
+                                result = d;


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

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.)

iSazonov · 2018-08-21T11:50:53Z

src/System.Management.Automation/engine/parser/tokenizer.cs

-                                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)


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

Hmmm. Agreed!

iSazonov · 2018-08-21T11:54:16Z

src/System.Management.Automation/engine/parser/tokenizer.cs

+                    {
+                        if (short.TryParse(strNum, style, NumberFormatInfo.InvariantInfo, out short s))
+                        {
+                            result = s * (short)multiplier;


What result do we expect for multiplier = 1Mb?

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.

Closed under next PR.

iSazonov · 2018-08-21T11:58:34Z

src/System.Management.Automation/engine/parser/tokenizer.cs

+                    }
+                    else if (suffix.HasFlag(NumberSuffixFlags.Unsigned) && ulong.TryParse(strNum, style, NumberFormatInfo.InvariantInfo, out ulong u))
+                    {
+                        u *= (ulong)multiplier;


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

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.

iSazonov · 2018-08-21T12:05:55Z

test/powershell/Language/Parser/Parser.Tests.ps1

-			@{ Script = "0x100000000"; Expected = [int64]0x100000000 }
+            @{ Script = "0x100000000"; Expected = [int64]0x100000000 }
+            #Tests for short notation
+            @{ Script = "10s"; Expected = "10" }


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

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

iSazonov · 2018-08-21T12:09:07Z

test/powershell/Language/Parser/Parser.Tests.ps1

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


Should we add 20x?

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. :)

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

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

iSazonov · 2018-08-21T12:10:53Z

test/powershell/Language/Parser/Parser.Tests.ps1

+            @{ Script = "21ss" }
+            @{ Script = "100ll" }
+            @{ Script = "150su" }
+            @{ Script = "10ds" }


Should we add 10ud?

Ah, good catch. Absolutely.

@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.

0x10d - have we tests for it?

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.

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

Added all these. :)

iSazonov · 2018-08-21T12:13:59Z

src/System.Management.Automation/engine/parser/tokenizer.cs

                        result = null;
                        return false;
                    }
+                    else if (suffix == NumberSuffixFlags.Short)


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

Again, agreed. Sorted out. :)

iSazonov · 2018-08-21T12:20:09Z

PowerShell Committee approved this in #7509.

Partially replace if stack with switches Add missing multiplier.

Apparently int16 * int16 always results in int32. Wat.

vexx32 · 2018-08-21T14:33:53Z

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!

…hods

vexx32 · 2018-09-18T08:33:13Z

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

How did that even happen, seriously...

Partially replace if stack with switches Add missing multiplier.

Apparently int16 * int16 always results in int32. Wat.

Teach me to try to restructure the test file too much! ;)

Fix test cases for reals with certain suffixes

No, really, I have no idea how the heck this happened. It's gone now.

Because I cannot find a distinct commit that actually modified this file

…werShell into Tokenizer/TypeSuffixes

vexx32 · 2018-09-18T09:13:11Z

Okay... @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. :)

iSazonov · 2018-09-18T09:47:48Z

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

vexx32 · 2018-09-18T17:11:33Z

@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

vexx32 added 8 commits August 12, 2018 17:44

Merge pull request #1 from PowerShell/master

f3b0413

Catch up to PowerShell/PowerShell/master

Merge pull request #2 from PowerShell/master

41e64d2

Catch up to master

Merge branch 'master' of git://github.com/PowerShell/PowerShell

8d4e16c

Add typesuffix traits to u and s

a636dca

Add type accelerators for uint, ulong, ushort, and short

4330f28

Add support for parsing uint, ulong, ushort, and short values

03739e1

Add tests for parsing the new literals

377f7b2

Add tests for new type accelerators

6f84b5a

vexx32 requested review from BrucePay and daxian-dbw as code owners August 20, 2018 16:57

Overlooked a suffix reference I need to change the type of.

1730a45

vexx32 mentioned this pull request Aug 20, 2018

Refactor the tokenizer #7509

Closed

8 tasks

Remove weird copy paste error

74c9c1a

How did that even happen, seriously...

iSazonov reviewed Aug 21, 2018

View reviewed changes

iSazonov self-assigned this Aug 21, 2018

iSazonov added the Committee-Reviewed PS-Committee has reviewed this and made a decision label Aug 21, 2018

iSazonov requested review from SteveL-MSFT and rjmholt August 21, 2018 12:21

vexx32 added 6 commits August 21, 2018 08:45

Address review comments

dd4c6c2

Partially replace if stack with switches Add missing multiplier.

Add More Tests for new literal types.

6d73617

Add comments for decimal weirdness & some tests

c37e531

Add test for ud suffix that should fail

3b058d7

Fix short parsing

a347629

Apparently int16 * int16 always results in int32. Wat.

Forgot the decimal case. Fixed that short parsing.

c628341

rjmholt added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Aug 21, 2018

Build framework for accepting and recognising named arguments for met…

4ffe824

…hods

vexx32 added 22 commits September 18, 2018 04:34

Add support for parsing uint, ulong, ushort, and short values

f59cc1e

Add tests for parsing the new literals

cf2e419

Remove weird copy paste error

01ad913

How did that even happen, seriously...

Address review comments

8135b27

Partially replace if stack with switches Add missing multiplier.

Add More Tests for new literal types.

7417725

Add comments for decimal weirdness & some tests

49212ac

Add test for ud suffix that should fail

e677fbb

Fix short parsing

7c2722c

Apparently int16 * int16 always results in int32. Wat.

Add clarifying comment to uint parsing cases

583287a

Reorder parsing cases for unsigned ints

8acc402

Revise parser notation tests

4736c18

Fix typo in test

d8cb6be

Expand numeric parsing test cases

920faca

Add explicit multiplier value tests

fb1f216

Fix Tests' expected values.

581c0dc

Cover *all* the test cases!

b711eb4

Fix testcases reference

fec8ef2

Teach me to try to restructure the test file too much! ;)

Fix typos in tests

bffa60d

Add test cases for numbers beginning with +

bb818d9

Fix test cases for reals with certain suffixes

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.

Manual revert ast.cs to original

cd2bc8e

Because I cannot find a distinct commit that actually modified this file

Merge branch 'Tokenizer/TypeSuffixes' of https://github.com/vexx32/Po…

a6eb532

…werShell into Tokenizer/TypeSuffixes

vexx32 mentioned this pull request Sep 18, 2018

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

Merged

10 tasks

vexx32 closed this Sep 18, 2018

vexx32 deleted the Tokenizer/TypeSuffixes branch September 19, 2018 22:59

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

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

Uh oh!

Conversation

vexx32 commented Aug 20, 2018

PR Summary

PR Checklist

Uh oh!

vexx32 commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vexx32 Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vexx32 Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Aug 21, 2018

Uh oh!

vexx32 commented Aug 21, 2018

Uh oh!

vexx32 commented Sep 18, 2018

Uh oh!

vexx32 commented Sep 18, 2018

Uh oh!

iSazonov commented Sep 18, 2018

Uh oh!

vexx32 commented Sep 18, 2018

Uh oh!

Reviewers

Assignees

Labels

vexx32 commented Aug 20, 2018 •

edited

Loading

vexx32 Aug 21, 2018 •

edited

Loading

vexx32 Aug 21, 2018 •

edited

Loading