RFC FS-1005 - Underscore literals #1243

Merged
merged 10 commits into from Jun 9, 2016

Conversation

Projects
None yet
7 participants
@AviAvni
Contributor

AviAvni commented Jun 3, 2016

I implemented the feature according to the RFC
I samples from RFC work properly please review before I added the tests

AviAvni added some commits Apr 26, 2016

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 3, 2016

Contributor

Can you please start adding the tests? Would make it much easier for me to review.

Contributor

forki commented Jun 3, 2016

Can you please start adding the tests? Would make it much easier for me to review.

@AviAvni

This comment has been minimized.

Show comment
Hide comment
@AviAvni

AviAvni Jun 3, 2016

Contributor

OK working on it thanks

Contributor

AviAvni commented Jun 3, 2016

OK working on it thanks

src/fsharp/FSharp.Core/prim-types.fs
@@ -4144,8 +4144,8 @@ namespace Microsoft.FSharp.Core
let inline ParseUInt16 (s:string) = (# "conv.ovf.u2" (ParseUInt32 s) : uint16 #)
let inline ParseIntPtr (s:string) = (# "conv.ovf.i" (ParseInt64 s) : nativeint #)
let inline ParseUIntPtr (s:string) = (# "conv.ovf.u" (ParseInt64 s) : unativeint #)
- let inline ParseDouble (s:string) = Double.Parse(s,NumberStyles.Float, CultureInfo.InvariantCulture)
- let inline ParseSingle (s:string) = Single.Parse(s,NumberStyles.Float, CultureInfo.InvariantCulture)
+ let inline ParseDouble (s:string) = Double.Parse(s.Replace("_", ""),NumberStyles.Float, CultureInfo.InvariantCulture)

This comment has been minimized.

@forki

forki Jun 3, 2016

Contributor

Why is there no trim in this place? Ideas?

@forki

forki Jun 3, 2016

Contributor

Why is there no trim in this place? Ideas?

This comment has been minimized.

@AviAvni

AviAvni Jun 3, 2016

Contributor

Why in the other places there is a trim?
As I see in the parser there are no leading or trailing whitespaces in numeric literals

@AviAvni

AviAvni Jun 3, 2016

Contributor

Why in the other places there is a trim?
As I see in the parser there are no leading or trailing whitespaces in numeric literals

src/fsharp/lex.fsl
@@ -83,6 +83,7 @@ let parseOctalUInt64 (s:string) p l =
parse p 0UL
let parseInt32 (s:string) =
+ let s = s.Replace("_", "")

This comment has been minimized.

@forki

forki Jun 3, 2016

Contributor

Can we make this a separate (inlined) helper function?

@forki

forki Jun 3, 2016

Contributor

Can we make this a separate (inlined) helper function?

This comment has been minimized.

@AviAvni

AviAvni Jun 3, 2016

Contributor

Done

@AviAvni

AviAvni Jun 3, 2016

Contributor

Done

@AviAvni

This comment has been minimized.

Show comment
Hide comment
@AviAvni

AviAvni Jun 3, 2016

Contributor

I added the positive test
for the negative it's look like I need vs15 to run them so i'm moving to the virtual machine

Contributor

AviAvni commented Jun 3, 2016

I added the positive test
for the negative it's look like I need vs15 to run them so i'm moving to the virtual machine

+let x7 = 0x5_2
+let x9 = 0_52
+let x10 = 05_2
+let x14 = 0o5_2

This comment has been minimized.

@forki

forki Jun 3, 2016

Contributor

I think we need tests that show that the values are actually the expected constants.

@forki

forki Jun 3, 2016

Contributor

I think we need tests that show that the values are actually the expected constants.

This comment has been minimized.

@AviAvni

AviAvni Jun 3, 2016

Contributor

Right sorry done

@AviAvni

AviAvni Jun 3, 2016

Contributor

Right sorry done

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 3, 2016

Contributor

added few comments. apart from that:

underscore

Contributor

forki commented Jun 3, 2016

added few comments. apart from that:

underscore

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 3, 2016

Contributor

somehow some tests are broken

[<Test>]
member this.ParseStringViaConversionOps() =
    let s : string = null
    CheckThrowsArgumentNullException2 "sbyte" (fun () -> sbyte s |> ignore)
    CheckThrowsArgumentNullException2 "byte" (fun () -> byte s |> ignore)
    CheckThrowsArgumentNullException2 "int16" (fun () -> int16 s |> ignore)
    CheckThrowsArgumentNullException2 "uint16 " (fun () -> uint16 s |> ignore)
    CheckThrowsArgumentNullException2 "int" (fun () -> int s |> ignore)
    CheckThrowsArgumentNullException2 "int32" (fun () -> int32 s |> ignore)
    CheckThrowsArgumentNullException2 "uint32" (fun () -> uint32 s |> ignore)
    CheckThrowsArgumentNullException2  "int64" (fun () -> int64 s |> ignore)
    CheckThrowsArgumentNullException2 "uint64" (fun () -> uint64 s |> ignore)
    CheckThrowsArgumentNullException2 "float32" (fun () -> float32 s |> ignore)
    CheckThrowsArgumentNullException2 "float" (fun () -> float s |> ignore)
    CheckThrowsArgumentNullException2 "decimal" (fun () -> decimal s |> ignore)

looks like it's related to parsing/converting nulls

Contributor

forki commented Jun 3, 2016

somehow some tests are broken

[<Test>]
member this.ParseStringViaConversionOps() =
    let s : string = null
    CheckThrowsArgumentNullException2 "sbyte" (fun () -> sbyte s |> ignore)
    CheckThrowsArgumentNullException2 "byte" (fun () -> byte s |> ignore)
    CheckThrowsArgumentNullException2 "int16" (fun () -> int16 s |> ignore)
    CheckThrowsArgumentNullException2 "uint16 " (fun () -> uint16 s |> ignore)
    CheckThrowsArgumentNullException2 "int" (fun () -> int s |> ignore)
    CheckThrowsArgumentNullException2 "int32" (fun () -> int32 s |> ignore)
    CheckThrowsArgumentNullException2 "uint32" (fun () -> uint32 s |> ignore)
    CheckThrowsArgumentNullException2  "int64" (fun () -> int64 s |> ignore)
    CheckThrowsArgumentNullException2 "uint64" (fun () -> uint64 s |> ignore)
    CheckThrowsArgumentNullException2 "float32" (fun () -> float32 s |> ignore)
    CheckThrowsArgumentNullException2 "float" (fun () -> float s |> ignore)
    CheckThrowsArgumentNullException2 "decimal" (fun () -> decimal s |> ignore)

looks like it's related to parsing/converting nulls

@AviAvni

This comment has been minimized.

Show comment
Hide comment
@AviAvni

AviAvni Jun 3, 2016

Contributor

Thanks you are great i'll check it tomorrow night

Contributor

AviAvni commented Jun 3, 2016

Thanks you are great i'll check it tomorrow night

src/fsharp/FSharp.Core/prim-types.fs
@@ -2504,10 +2504,13 @@ namespace Microsoft.FSharp.Core
let rec parse n acc = if n < l then parse (n+1) (acc *.. 2UL +.. (match s.Chars(n) with '0' -> 0UL | '1' -> 1UL | _ -> formatError())) else acc in
parse p 0UL
+ let inline removeUnderscores (s:string) =
+ s.Replace("_", "")

This comment has been minimized.

@forki

forki Jun 3, 2016

Contributor

I think this needs a null check to fix the tests.

match s with
| null -> s
| _ -> s.Replace("_", "")

Same with other instance of this method in lex.fsl

@forki

forki Jun 3, 2016

Contributor

I think this needs a null check to fix the tests.

match s with
| null -> s
| _ -> s.Replace("_", "")

Same with other instance of this method in lex.fsl

@AviAvni

This comment has been minimized.

Show comment
Hide comment
@AviAvni

AviAvni Jun 5, 2016

Contributor

@dotnet-bot test this please

Contributor

AviAvni commented Jun 5, 2016

@dotnet-bot test this please

@AviAvni

This comment has been minimized.

Show comment
Hide comment
@AviAvni

AviAvni Jun 8, 2016

Contributor

@dsyme what do you think about the implementation?
the errors message for invalid literals in the RFC are the errors the compile need to report or the
regular error for invalid numeric literals is ok?
I have a problem to add test to the errors it's tell me the error no match and then print the same error

Contributor

AviAvni commented Jun 8, 2016

@dsyme what do you think about the implementation?
the errors message for invalid literals in the RFC are the errors the compile need to report or the
regular error for invalid numeric literals is ok?
I have a problem to add test to the errors it's tell me the error no match and then print the same error

src/fsharp/lex.fsl
@@ -82,7 +82,13 @@ let parseOctalUInt64 (s:string) p l =
let rec parse n acc = if n < l then parse (n+1) (acc * 8UL + (let c = s.[n] in if c >= '0' && c <= '7' then Convert.ToUInt64 c - Convert.ToUInt64 '0' else formatError())) else acc
parse p 0UL
+let inline removeUnderscores (s:string) =

This comment has been minimized.

@dsyme

dsyme Jun 9, 2016

Contributor

There's no need to mark this "inline" though it's prety harmless

@dsyme

dsyme Jun 9, 2016

Contributor

There's no need to mark this "inline" though it's prety harmless

src/fsharp/lex.fsl
@@ -175,11 +181,12 @@ let anychar = [^'\n''\r']
let anystring = anychar*
let op_char = '!'|'$'|'%'|'&'|'*'|'+'|'-'|'.'|'/'|'<'|'='|'>'|'?'|'@'|'^'|'|'|'~'|':'
let ignored_op_char = '.' | '$' | '?'
+let seperator = '_'

This comment has been minimized.

@dsyme

dsyme Jun 9, 2016

Contributor

spelling is "separator" though it's no big deal :)

@dsyme

dsyme Jun 9, 2016

Contributor

spelling is "separator" though it's no big deal :)

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jun 9, 2016

Contributor

LGTM - I think this is ready to go.

Contributor

dsyme commented Jun 9, 2016

LGTM - I think this is ready to go.

@dsyme dsyme changed the title from WIP - Underscore literals to RFC FS-1005 - Underscore literals Jun 9, 2016

AviAvni added some commits Jun 9, 2016

@AviAvni

This comment has been minimized.

Show comment
Hide comment
@AviAvni

AviAvni Jun 9, 2016

Contributor

@dsyme thanks I fixed the typo and inline issues

Contributor

AviAvni commented Jun 9, 2016

@dsyme thanks I fixed the typo and inline issues

@KevinRansom KevinRansom merged commit bfc92fb into Microsoft:master Jun 9, 2016

4 checks passed

Windows_NT Debug Build Build finished.
Details
Windows_NT Release_ci_part1 Build Build finished.
Details
Windows_NT Release_ci_part2 Build Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@khyperia

This comment has been minimized.

Show comment
Hide comment
@khyperia

khyperia Jun 9, 2016

@AviAvni You seem to be missing a test case for the following bit of the spec:

You cannot place underscores in the following places:

  • Prior to an F or L or other suffix

and I noticed that the implementation grammar seems to allow it (1.0_F), although I can't test it myself at the moment. Would you mind adding a test case for that?

khyperia commented Jun 9, 2016

@AviAvni You seem to be missing a test case for the following bit of the spec:

You cannot place underscores in the following places:

  • Prior to an F or L or other suffix

and I noticed that the implementation grammar seems to allow it (1.0_F), although I can't test it myself at the moment. Would you mind adding a test case for that?

@khyperia

This comment has been minimized.

Show comment
Hide comment
@khyperia

khyperia Jun 9, 2016

Also, the spec doesn't seem to make any stance on the validity of underscores around the exponent notation (2.1_e2F, 2.1e_2F) - I think the grammar currently allows it, although it seems consistent to disallow it (for example, C#7's similar feature disallows underscores around an exponent). In any case, we probably want to add a test asserting one behavior or another.

khyperia commented Jun 9, 2016

Also, the spec doesn't seem to make any stance on the validity of underscores around the exponent notation (2.1_e2F, 2.1e_2F) - I think the grammar currently allows it, although it seems consistent to disallow it (for example, C#7's similar feature disallows underscores around an exponent). In any case, we probably want to add a test asserting one behavior or another.

@AviAvni

This comment has been minimized.

Show comment
Hide comment
@AviAvni

AviAvni Jun 10, 2016

Contributor

@khyperia thanks I opened new pr fixed the float issue and add tests for errors #1256

Contributor

AviAvni commented Jun 10, 2016

@khyperia thanks I opened new pr fixed the float issue and add tests for errors #1256

+if socialSecurityNumber <> 999999999L then
+ failwith "Wrong parsing"
+
+let pi = 3.14_15F

This comment has been minimized.

@jcouv

jcouv Jun 13, 2016

Member

spacing is odd

@jcouv

jcouv Jun 13, 2016

Member

spacing is odd

This comment has been minimized.

@forki

forki Sep 6, 2016

Contributor

I think that's by design to test more interesting cases.

@forki

forki Sep 6, 2016

Contributor

I think that's by design to test more interesting cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment