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 NoEquality to range #6610

Closed
wants to merge 2 commits into from
Closed

Add NoEquality to range #6610

wants to merge 2 commits into from

Conversation

cartermp
Copy link
Contributor

@cartermp cartermp commented Apr 22, 2019

This is to address #6052

It's quite a bit of machinery that I think might actually be better served by having the compiler properly handle equality and comparison for structs with custom equality. But that could also be a breaking change IIRC - @TIHan @dsyme is that accurate?

Some notes:

  • Lack of ability to pass in a custom comparer to List/Seq/Array combinators is why there is duplication going on here - should there be a core library change to support something like this? That would allow for far less code additions and open up doing the same for List, which is currently blocked because the actual implementation ultimately relies on inline IL which is only available in FSharp.Core.
  • Some of this is pretty ugly; I imagine it could get cleaned up

@dsyme
Copy link
Contributor

dsyme commented Apr 23, 2019

Given the amount of code and complexity being added I'd rather take a look at addressing the core problems here - it should be totally fine to use CustomEquality on a struct like range and void boxing.

I'm sure we can address this tactically for the actual allocations you're seeing once we have the relevant stacks. But it shouldn't be as much code as this is.

@manofstick's PRs are also relevant here and fix the problem of boxing much more universally for structs.

@cartermp
Copy link
Contributor Author

I agree, I'd definitely prefer that then have all of this machinery and two versions of standard combinators just to get around what should be considered a bug in the compiler.

@TIHan had some interesting thoughts about compatibility that are worth considering here, it sounds like something we'd have to be very judicious with changing.

Separately, digging through illib.fs and lib.fs was kind of painful. illib.fs clearly has many more utilities than just what is needed for manipulating IL, and lib.fs just seems like a smaller version of library functions that might have just been abandoned at some point. There's clearly a need for a "compiler utility library" where things like additional Seq functions can live. What are your thoughts on factoring all of that out?

@dsyme
Copy link
Contributor

dsyme commented Apr 23, 2019

Separately, digging through illib.fs and lib.fs was kind of painful. illib.fs clearly has many more utilities than just what is needed for manipulating IL, and lib.fs just seems like a smaller version of library functions that might have just been abandoned at some point. There's clearly a need for a "compiler utility library" where things like additional Seq functions can live. What are your thoughts on factoring all of that out?

Factoring into a separate assembly would not be right. Merging lib.fs into illib.fs and ensuring uniform design makes sense. The split was historical.

I don't mind if illib.fs gets split into multiple files too though you always end up with a bucket-of-stuff file in the end.

@cartermp
Copy link
Contributor Author

Factoring into a separate assembly would not be right.

What's the rationale? I understand having utilities specific to IL generation here, but there's so much generic utility code here - string utilities and active patterns, Cancellable computation expression, graph data structures, etc. I could imagine some of it being utilized by other projects. For example, Extensions.fs in FSharp.Editor also defines an equivalent Option.attempt function. There may be more, but I haven't exhaustively looked.

@dsyme
Copy link
Contributor

dsyme commented Apr 24, 2019

What's the rationale? I understand having utilities specific to IL generation here, but there's so much generic utility code here - string utilities and active patterns, Cancellable computation expression, graph data structures, etc. I could imagine some of it being utilized by other projects. For example, Extensions.fs in FSharp.Editor also defines an equivalent Option.attempt function. There may be more, but I haven't exhaustively looked.

People are welcome to copy it out to a separate project in fsprojects or elsewhere. But the scope of illib.fs+lib.fs is precisely what's needed by the F# compiler - nothing more and nothing less - if a utility function is not needed, we will delete it, and if some random thing is needed, we will add it. There is not a coherent, well-designed library here, it's just a bucket of utility code.

@cartermp cartermp closed this May 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants