RFC FS-1007: Additions to `Option` module #1781

neoeinstein · 2016-11-18T22:48:33Z

This PR provides an implementation for RFC FS-1007 and is discussed in fslang-design.

Added functions:

val defaultIfNone :              'a         -> 'a option -> 'a
val defaultIfNoneFrom : (unit -> 'a)        -> 'a option -> 'a
val orElse :                     'a option  -> 'a option -> 'a option
val orElseFrom :        (unit -> 'a option) -> 'a option -> 'a option

val map2: ('a -> 'b -> 'c)       -> 'a option -> 'b option -> 'c option
val map3: ('a -> 'b -> 'c -> 'd) -> 'a option -> 'b option -> 'c option -> 'd option

val apply: 'a option -> ('a -> 'b) option -> 'b option

val contains: 'a -> 'a option -> bool

val flatten: 'a option option -> 'a option

This PR includes and supersedes #1636.

Per discussions in fsharp/fslang-design#60. Adds `defaultIfNone`, `defaultIfNoneFrom`, `orElse`, and `orElseFrom`.

`src/fsharp/FSharp.Core.Unittests/SurfaceArea.net20.fs` was removed.

Per RFC-FS-1007 and the discussion in fsharp/fslang-design#60

Per RFC FS-1007 and the discussion in fsharp/fslang-design#60

msftclas · 2016-11-18T22:48:36Z

Hi @neoeinstein, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

KevinRansom · 2016-11-21T22:26:20Z

src/fsharp/FSharp.Core/option.fs

@@ -16,6 +16,18 @@ namespace Microsoft.FSharp.Core
        [<CompiledName("IsNone")>]
        let inline isNone option = match option with None -> true | Some _ -> false

+        [<CompiledName("DefaultIfNone")>]
+        let defaultIfNone def option = match option with None -> def | Some v -> v


probably inline. I'm guessing this will be the most commonly used of these.

For use cases like this, is it more appropriate to give the JIT a hint to do the inlining with AggressiveInlining? I see the case for inline on contains, but still a bit unsure for defaultValue and orElse.

KevinRansom · 2016-11-21T22:26:47Z

src/fsharp/FSharp.Core/option.fs

+        let defaultIfNoneFrom defThunk option = match option with None -> defThunk () | Some v -> v
+
+        [<CompiledName("OrElse")>]
+        let orElse ifNone option = match option with None -> ifNone | Some _ -> option


Probably inline.

KevinRansom · 2016-11-21T22:27:16Z

src/fsharp/FSharp.Core/option.fs

@@ -31,15 +43,39 @@ namespace Microsoft.FSharp.Core
        [<CompiledName("ForAll")>]
        let forall p inp = match inp with None -> true | Some x -> p x

+        [<CompiledName("Contains")>]
+        let contains x inp = match inp with None -> false | Some v -> v = x


Inline I would think. Quite common too.

I've been considering what the relative benefits of inline are to letting JIT handle it. I've posted a gist with the IL that was generated on my computer for the Option module. Most of the functions are less than 32 bytes of IL, with the only exceptions being get, map2, and map3, which makes them largely eligible for JIT-inlining. What incremental benefit does the inline keyword provide in this case?

What incremental benefit does the inline keyword provide in this case?

Generic equality is used in the body. That's expensive, but becomes cheap if type-specialized. Definitely an incentive to inline.

That's exactly the information I was looking for. I'll add inlines appropriate to this advice.

Inline for contains has been added.

dsyme · 2016-11-22T12:15:56Z

I gave some feedback on the RFC here: fsharp/fslang-design#60 (comment). Just a couple of naming changes requested

@dsyme

Per @dsyme, generic equality is expensive but with inlining, the compiler can type-specialize the call, making it much cheaper.

KevinRansom · 2016-12-13T20:23:22Z

@dotnet-bot test this please

KevinRansom · 2016-12-14T20:51:01Z

@dotnet-bot test this please

KevinRansom · 2016-12-17T07:09:11Z

@neoeinstein
Thank you for these great additions

Kevin

andredublin and others added 12 commits October 12, 2016 17:58

init of new option module functions

8d7bd90

add xml doc to fsi, use defaultArg instead of match

c991fbc

update signature

53e774e

Merge branch 'master' into RFC-FS-1007

edb1f10

Update to Option module signatures

6d951a0

Per discussions in fsharp/fslang-design#60. Adds `defaultIfNone`, `defaultIfNoneFrom`, `orElse`, and `orElseFrom`.

Update surface area with new functions

172a791

Add tests for Option map2 and map3

bc9e832

Fix merge conflict with upstream

b048ad1

`src/fsharp/FSharp.Core.Unittests/SurfaceArea.net20.fs` was removed.

Add Option.contains

03d2035

Per RFC-FS-1007 and the discussion in fsharp/fslang-design#60

Add Option.flatten

89832a3

Per RFC FS-1007 and the discussion in fsharp/fslang-design#60

Merge branch 'RFC-FS-1007-contains' into RFC-FS-1007

68c445f

Merge branch 'RFC-FS-1007-flatten' into RFC-FS-1007

8492849

msftclas added the cla-already-signed label Nov 18, 2016

This was referenced Nov 18, 2016

RFC FS-1007: Add Option.contains #1778

Closed

RFC FS-1007: Add Option.flatten #1779

Closed

RFC FS-1007: Add Option.apply #1780

Closed

[WIP] RFC FS-1007 - Additional Option module functions #1636

Closed

neoeinstein changed the title ~~RFC FS-1007~~ RFC FS-1007: Additions to Option module Nov 18, 2016

KevinRansom reviewed Nov 21, 2016

View reviewed changes

Marcus Griep added 3 commits November 22, 2016 12:31

Add tests on Option module properties

affe54c

Update naming of Option members

014ef71

Update surface area tests

3ee1b42

neoeinstein force-pushed the RFC-FS-1007 branch from 8d97108 to 3ee1b42 Compare November 22, 2016 17:54

neoeinstein mentioned this pull request Nov 22, 2016

[RFC FS-1007 Discussion] Additional Option module functions fsharp/fslang-design#60

Closed

Inline Option.contains

060db6a

Per @dsyme, generic equality is expensive but with inlining, the compiler can type-specialize the call, making it much cheaper.

KevinRansom approved these changes Dec 17, 2016

View reviewed changes

KevinRansom merged commit ca2ed9b into dotnet:master Dec 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC FS-1007: Additions to `Option` module #1781

RFC FS-1007: Additions to `Option` module #1781

neoeinstein commented Nov 18, 2016

msftclas commented Nov 18, 2016

KevinRansom Nov 21, 2016

neoeinstein Nov 23, 2016

KevinRansom Nov 21, 2016

KevinRansom Nov 21, 2016

neoeinstein Nov 22, 2016 •

edited

Loading

dsyme Nov 22, 2016

neoeinstein Nov 22, 2016

neoeinstein Nov 23, 2016

dsyme commented Nov 22, 2016

KevinRansom commented Dec 13, 2016

KevinRansom commented Dec 14, 2016

KevinRansom commented Dec 17, 2016

RFC FS-1007: Additions to Option module #1781

RFC FS-1007: Additions to Option module #1781

Conversation

neoeinstein commented Nov 18, 2016

msftclas commented Nov 18, 2016

KevinRansom Nov 21, 2016

Choose a reason for hiding this comment

neoeinstein Nov 23, 2016

Choose a reason for hiding this comment

KevinRansom Nov 21, 2016

Choose a reason for hiding this comment

KevinRansom Nov 21, 2016

Choose a reason for hiding this comment

neoeinstein Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

dsyme Nov 22, 2016

Choose a reason for hiding this comment

neoeinstein Nov 22, 2016

Choose a reason for hiding this comment

neoeinstein Nov 23, 2016

Choose a reason for hiding this comment

dsyme commented Nov 22, 2016

KevinRansom commented Dec 13, 2016

KevinRansom commented Dec 14, 2016

KevinRansom commented Dec 17, 2016

RFC FS-1007: Additions to `Option` module #1781

RFC FS-1007: Additions to `Option` module #1781

neoeinstein Nov 22, 2016 •

edited

Loading