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 support for Byte Literals #7901

Merged
merged 10 commits into from
Oct 11, 2018
Merged

Conversation

vexx32
Copy link
Collaborator

@vexx32 vexx32 commented Oct 1, 2018

PR Summary

  • Adds y suffix that is used to specify a numeric literal as the sbyte data type.
    • Can be combined with the existing u suffix as uy to specify the byte data type
  • Adds tests to ensure the parsing for these works correctly.

Examples:

PS> 100y
100
PS> 100y.GetType().FullName
System.SByte
PS> 100uy
100
100uy.GetType().FullName
System.Byte
PS> 300uy
# Error: Invalid numeric literal

N.B.: I believe this will be a breaking change, as it means some command names (specifically, numerals suffixed with a y such as 100y) will now be interpreted as numerals instead of potential function or command names.

Ref: #7557 -- This will resolve the first part of that issue. The second portion of that issue will be solved in a follow-up PR where binary literal support is added.

PR Checklist

@iSazonov iSazonov added Breaking-Change breaking change that may affect users Documentation Needed in this repo Documentation is needed in this repo labels Oct 1, 2018
@iSazonov iSazonov changed the title Tokenizer Support for Byte Literals Add support for Byte Literals Oct 1, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Oct 1, 2018

@vexx32 Please open Issue in docs repo and add link to the PR description

@vexx32
Copy link
Collaborator Author

vexx32 commented Oct 2, 2018

CodeFactor issues remaining are just in missing documentation in the enum; I can either add that to this PR or address it in follow-up PR with the rest of the changes. I would prefer to keep this one minimal, however. 😄

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 3, 2018

@vexx32 Feel free to address CodeFactor issues.

@vexx32
Copy link
Collaborator Author

vexx32 commented Oct 3, 2018

Rebased for CIs && added <summary> headers for CodeFactor 😄

Use specific combined flag names in parsing cases
Add (s)byte parsing cases for TryGetNumberValue
Remove impossible tests
Rework byte parsing shortcut

Fix failing test scenario caused by hex parse handling of multiplier

Simplify return by ignoring multuplier for (s)byte-suffixed values

Add comment for negative multiplier possibility

Still need to utilise multiplier for signed byte case
Multiplier can determine negative in signed byte cases with hex

Adjust byte parse shortcuts
Add comments
Add summary headers to enum and values
@iSazonov
Copy link
Collaborator

iSazonov commented Oct 4, 2018

@daxian-dbw Could you please review the next step PR? After that we could continue and implement previously planned performance optimizations for the code in follow PR.

@iSazonov iSazonov self-assigned this Oct 8, 2018
@TravisEz13 TravisEz13 added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Oct 8, 2018
@vexx32
Copy link
Collaborator Author

vexx32 commented Oct 8, 2018

@TravisEz13 you fellas are free to pore over it one more time, but just in case you weren't aware / didn't recall, it has been discussed a good bit and reviewed by the committee in terms of the concept over in #7557 😄

@SteveL-MSFT
Copy link
Member

@TravisEz13 is there something specific you need the @PowerShell/powershell-committee to review for this PR?

@TravisEz13
Copy link
Member

@SteveL-MSFT This is marked as a breaking change. The issue linked is not marked as reviewed. Looking back at the issue, it looks like this portion of the design was reviewed. Is that correct?

@vexx32
Copy link
Collaborator Author

vexx32 commented Oct 8, 2018

@TravisEz13 sorry for the confusion; I requested the committee further review the ensuing discussion regarding possibly supporting underscore literal syntax and native BigInteger values. This portion was reviewed, aye. 😃

@TravisEz13 TravisEz13 added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 8, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Oct 9, 2018

@daxian-dbw Could you please look the PR before we merge?

@iSazonov iSazonov merged commit e660e96 into PowerShell:master Oct 11, 2018
@joeyaiello joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018
@vexx32 vexx32 deleted the Tokenizer/ByteLiterals branch January 22, 2019 13:20
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 1, 2019
adityapatwardhan pushed a commit to adityapatwardhan/PowerShell that referenced this pull request Apr 9, 2019
- Adds y suffix that is used to specify a numeric literal as the sbyte data type. 
- Can be combined with the existing u suffix as uy to specify the byte data type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking-Change breaking change that may affect users CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants