Allow digit separator after base specifier by alrz · Pull Request #20449 · dotnet/roslyn

alrz · 2017-06-26T17:50:00Z

Proposal: dotnet/csharplang#65

jcouv · 2017-06-29T20:09:59Z

Ping @gafter since he's the LDM champion for that proposal.

jcouv · 2017-08-01T15:13:14Z

@gafter Did we get approval from LDM on this already?

jcouv · 2017-08-04T16:59:26Z

Neal confirmed that LDM approved the feature. Let's go ahead with review.

jcouv · 2017-08-04T17:01:24Z

src/Compilers/CSharp/Portable/Parser/Lexer.cs

-                underscoreInWrongPlace = true;
+                if (isHex || isBinary)
+                {
+                    // TODO(leading-digit-separator): check feature flag


We don't let new TODOs into the master branch.
Blockers should be addressed. Non-blockers should be tracked with a github issue.

jcouv

Please address the TODOs (ie adding a feature flag). Thanks

…git-separator

alrz · 2017-08-07T20:17:08Z

failure: Microsoft.CodeAnalysis.Editor.UnitTests.Structure.StructureTaggerTests.OutliningTaggerTooltipText (details)

FYI @dotnet/roslyn-infrastructure

jcouv · 2017-08-07T22:22:51Z

    // Allows underscores in integers, except at beginning and end

Please update comment #Resolved

Refers to: src/Compilers/CSharp/Portable/Parser/Lexer.cs:935 in ee697a5. [](commit_id = ee697a5, deletion_comment = False)

jcouv · 2017-08-07T22:24:54Z

src/Compilers/CSharp/Test/Syntax/LexicalAndXml/LexicalTests.cs


+        [Fact]
+        [Trait("Feature", "Literals")]
+        public void TestNumericWithLeadingUnderscores()


Please test with multiple underscores. @gafter can confirm expected language/spec behavior. #Resolved

Also test for trailing underscore: "0x_"

In reply to: 131779530 [](ancestors = 131779530)

jcouv · 2017-08-07T22:26:23Z

src/Compilers/CSharp/Portable/Parser/Lexer.cs

-                underscoreInWrongPlace = true;
+                if (isHex || isBinary)
+                {
+                    CheckFeatureAvailability(MessageID.IDS_FeatureLeadingDigitSeparator);


Should lastCharWasUnderscore be set to true?
Test 0x_1 with language version prior to IDS_FeatureDigitSeparator. I think usedUnderscore should be left unset (no need for cascading feature error)

jcouv · 2017-08-07T22:34:05Z

src/Compilers/VisualBasic/Test/Syntax/Scanner/ScannerTests.vb

+        Assert.Equal(SyntaxKind.IntegerLiteralToken, tk.Kind)
+        Assert.Equal(LiteralBase.Octal, tk.GetBase())
+        Assert.Equal(&O1, tk.Value)
+        Assert.Equal(" &O_1 ", tk.ToFullString())


Same test suggestions for VB. Thanks #Resolved

jcouv

Ping the roslyn-compiler alias for review when feedback addressed. I will be away for some time.
Thanks

alrz · 2017-08-10T17:47:12Z

@jcouv I can't tag @dotnet/roslyn-compiler. also tagging @gafter for review.

jcouv · 2017-08-10T20:38:21Z

@dotnet/roslyn-compiler for review (community contribution, approved by LDM already). Thanks

Out of office

cston · 2017-08-11T15:58:55Z

src/Compilers/CSharp/Portable/Parser/Lexer.cs

Consider switching order: if (firstCharWasUnderscore) { ... } else if (lastCharWasUnderscore) { ... }.
#Resolved

cston · 2017-08-11T16:18:43Z

src/Compilers/CSharp/Test/Syntax/LexicalAndXml/LexicalTests.cs

Please add tests for 1E+_2, 1E-_2, and 1E_. #Resolved

cston · 2017-08-11T16:22:31Z

src/Compilers/VisualBasic/Portable/Scanner/Scanner.vb

Unnecessary parentheses around entire Boolean expression, here and below.

cston · 2017-08-11T16:33:18Z

src/Compilers/VisualBasic/Portable/Scanner/Scanner.vb

Consider assigning False here since the value is not explicitly initialized any more. Also, there are several cases below where the value is assigned UnderscoreInWrongPlace = UnderscoreInWrongPlace Or (Peek(Here - 1) = "_"c) where UnderscoreInWrongPlace is initially False. (In short, the UnderscoreInWrongPlace Or is unnecessary.)

In short, the UnderscoreInWrongPlace Or is unnecessary.

Yeah, crossed my mind, shouldn't VB flag those cases in the first place?

cston · 2017-08-11T16:40:46Z

src/Compilers/VisualBasic/Portable/Scanner/Scanner.vb

With ElseIf rather than If we're no longer reporting both overflow and feature-not-available errors (e.g.: compiling Dim x = &H123_456_789_ABC_DEF_123 with -langversion:14). We should report both errors in such cases since the errors are independent.

alrz · 2017-08-11T18:59:00Z

src/Compilers/VisualBasic/Portable/Scanner/Scanner.vb

@@ -2085,13 +2095,16 @@ FullWidthRepeat2:

            If Overflows Then


FYI The order is now in sync with C#, an interesting case is an overflowing integer with a trailing underscore which gives 1 and 2 errors in VB and C# respectively. This is now fixed.

Any other case that needs to be verified here?

jcouv

LGTM Thanks
Ping @cston

jcouv · 2017-08-30T21:26:16Z

Merged. Thanks @alrz

For cleaner history, it would have been even better to rebase and squash your branch, rather than merging latest master. But given that the PR only has a couple of commit, it's not crucial (I merged without squashing, because of the merge from master).

dnfclas added the cla-already-signed label Jun 26, 2017

Allow digit separator after base specifier

8d7fb2b

alrz force-pushed the features/leading-digit-separator branch 2 times, most recently from a7227bc to 8d7fb2b Compare June 26, 2017 18:25

Pilchie added the Area-Compilers label Jun 27, 2017

jcouv added Area-Language Design Language-C# labels Jun 29, 2017

jcouv assigned gafter Jun 29, 2017

jcouv added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Aug 1, 2017

jcouv reviewed Aug 4, 2017

View reviewed changes

jcouv previously requested changes Aug 4, 2017

View reviewed changes

alrz added 2 commits August 5, 2017 00:40

Merge remote-tracking branch 'origin/master' into features/leading-di…

75365a4

…git-separator

Add feature flags

ee697a5

jcouv reviewed Aug 7, 2017

View reviewed changes

jcouv self-assigned this Aug 9, 2017

cston reviewed Aug 11, 2017

View reviewed changes

Do not cascade feature errors

b268dc1

alrz force-pushed the features/leading-digit-separator branch from ece5c60 to a1395e3 Compare August 11, 2017 16:19

cston reviewed Aug 11, 2017

View reviewed changes

alrz force-pushed the features/leading-digit-separator branch from a1395e3 to 66f98ba Compare August 11, 2017 18:40

PR feedback

4bad4d7

alrz force-pushed the features/leading-digit-separator branch from 66f98ba to 4bad4d7 Compare August 11, 2017 18:56

alrz commented Aug 11, 2017

View reviewed changes

jcouv approved these changes Aug 30, 2017

View reviewed changes

Merge branch 'master' into features/leading-digit-separator

2726b4c

gafter mentioned this pull request Aug 30, 2017

Champion "allow digit separator after 0b or 0x" (C# 7.2, VB 15.5) dotnet/csharplang#65

Open

5 tasks

cston approved these changes Aug 30, 2017

View reviewed changes

jcouv merged commit 7dd7af9 into dotnet:master Aug 30, 2017

alrz deleted the features/leading-digit-separator branch August 30, 2017 21:36

sharwell added Community The pull request was submitted by a contributor who is not a Microsoft employee. and removed Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Aug 31, 2017

jcouv added this to the 15.5 milestone Sep 2, 2017

dgrunwald mentioned this pull request Nov 15, 2017

C# language support status icsharpcode/ILSpy#829

Open

Conversation

alrz commented Jun 26, 2017

Uh oh!

jcouv commented Jun 29, 2017

Uh oh!

jcouv commented Aug 1, 2017

Uh oh!

jcouv commented Aug 4, 2017

Uh oh!

jcouv Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

alrz commented Aug 7, 2017 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Aug 7, 2017 • 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!

jcouv Aug 7, 2017

Choose a reason for hiding this comment

Uh oh!

jcouv Aug 7, 2017 • 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!

jcouv Aug 7, 2017 • 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!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

alrz commented Aug 10, 2017

Uh oh!

jcouv commented Aug 10, 2017

Uh oh!

cston Aug 11, 2017 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Aug 11, 2017 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Aug 11, 2017

Choose a reason for hiding this comment

Uh oh!

cston Aug 11, 2017

Choose a reason for hiding this comment

Uh oh!

alrz Aug 11, 2017

Choose a reason for hiding this comment

Uh oh!

cston Aug 11, 2017

Choose a reason for hiding this comment

Uh oh!

alrz Aug 11, 2017 • 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!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Aug 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alrz commented Aug 7, 2017 •

edited by jcouv

Loading

jcouv commented Aug 7, 2017 •

edited

Loading

jcouv Aug 7, 2017 •

edited

Loading

jcouv Aug 7, 2017 •

edited

Loading

jcouv Aug 7, 2017 •

edited

Loading

cston Aug 11, 2017 •

edited by jcouv

Loading

cston Aug 11, 2017 •

edited by jcouv

Loading

alrz Aug 11, 2017 •

edited

Loading