Allow digit separator after base specifier #20449

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

@@ -2616,6 +2620,52 @@ public void TestNumericWithUnderscoresWithoutFeatureFlag()

        [Fact]
        [Trait("Feature", "Literals")]
+        public void TestNumericWithLeadingUnderscores()
+        {
+            var text = "0x_A";


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

+                CheckFeatureAvailability(MessageID.IDS_FeatureLeadingDigitSeparator);
+                // No need for cascading feature error
+                usedUnderscore = false;
+            }


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

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

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

@@ -2695,17 +2772,6 @@ public void TestNumericWithBadUnderscores()
            Assert.Equal("error CS1013: Invalid number", errors[0].ToString(EnsureEnglishUICulture.PreferredOrNull));
            Assert.Equal(text, token.Text);


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

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

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

@@ -1695,7 +1696,10 @@ FullWidthRepeat:
                        IntegerLiteralStart = Here
                        Base = LiteralBase.Hexadecimal

-                        UnderscoreInWrongPlace = (CanGet(Here) AndAlso Peek(Here) = "_"c)
+                        If (CanGet(Here) AndAlso Peek(Here) = "_"c) Then


Unnecessary parentheses around entire Boolean expression, here and below.

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

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

@@ -1673,6 +1673,7 @@ FullWidthRepeat:
            Dim IntegerLiteralStart As Integer
            Dim UnderscoreInWrongPlace As Boolean


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

-            End If
-
-            If UnderscoreUsed Then
+            ElseIf LeadingUnderscoreUsed Then


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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow digit separator after base specifier #20449

Allow digit separator after base specifier #20449

alrz commented Jun 26, 2017

jcouv commented Jun 29, 2017

jcouv commented Aug 1, 2017

jcouv commented Aug 4, 2017

jcouv Aug 4, 2017

jcouv left a comment

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

jcouv Aug 7, 2017 •

edited

Loading

jcouv Aug 7, 2017 •

edited

Loading

jcouv left a comment

alrz commented Aug 10, 2017

jcouv commented Aug 10, 2017

cston Aug 11, 2017 •

edited by jcouv

Loading

cston Aug 11, 2017 •

edited by jcouv

Loading

cston Aug 11, 2017

cston Aug 11, 2017

alrz Aug 11, 2017

cston Aug 11, 2017

alrz Aug 11, 2017 •

edited

Loading

jcouv left a comment

jcouv commented Aug 30, 2017

		@@ -2695,17 +2772,6 @@ public void TestNumericWithBadUnderscores()
		Assert.Equal("error CS1013: Invalid number", errors[0].ToString(EnsureEnglishUICulture.PreferredOrNull));
		Assert.Equal(text, token.Text);

		@@ -1673,6 +1673,7 @@ FullWidthRepeat:
		Dim IntegerLiteralStart As Integer
		Dim UnderscoreInWrongPlace As Boolean

Allow digit separator after base specifier #20449

Allow digit separator after base specifier #20449

Conversation

alrz commented Jun 26, 2017

jcouv commented Jun 29, 2017

jcouv commented Aug 1, 2017

jcouv commented Aug 4, 2017

jcouv Aug 4, 2017

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

alrz commented Aug 7, 2017 • edited by jcouv Loading

jcouv commented Aug 7, 2017 • edited Loading

jcouv Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv Aug 7, 2017

Choose a reason for hiding this comment

jcouv Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

alrz commented Aug 10, 2017

jcouv commented Aug 10, 2017

cston Aug 11, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Aug 11, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Aug 11, 2017

Choose a reason for hiding this comment

cston Aug 11, 2017

Choose a reason for hiding this comment

alrz Aug 11, 2017

Choose a reason for hiding this comment

cston Aug 11, 2017

Choose a reason for hiding this comment

alrz Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented Aug 30, 2017

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