Skip to content

Conversation

@Happypig375
Copy link
Member

It's resolvable by GenericOne already. Why not add it to GenericZero?

@cartermp cartermp requested a review from dsyme January 21, 2020 18:47
@cartermp
Copy link
Contributor

Needs a @dsyme review since I don't have context on this omission

@dsyme
Copy link
Contributor

dsyme commented Jan 21, 2020

This is by design, though I don't feel strongly about it - the rationale is

  1. if you really want a numeric type you should use UInt16.

  2. The One on char is only supported to allow addition in the range operator for x in 'a' .. 'z' do ...

Is there an example which shows the good reason to support Zero on Char?

See also ConstraintSolver.fs, which would also need updating, and we would need testing of course.

      // We pretend for uniformity that the numeric types have a static property called Zero and One 
      // As with constants, only zero is polymorphic in its units
      | [], [ty], false, "get_Zero", [] 
          when IsNumericType g ty -> 
          do! SolveTypeEqualsTypeKeepAbbrevs csenv ndeep m2 trace rty ty
          return TTraitBuiltIn

      | [], [ty], false, "get_One", [] 
          when IsNumericType g ty || isCharTy g ty -> 
          do! SolveDimensionlessNumericType csenv ndeep m2 trace ty 
          do! SolveTypeEqualsTypeKeepAbbrevs csenv ndeep m2 trace rty ty
          return TTraitBuiltIn

@Happypig375
Copy link
Member Author

It's mostly for consistency. It is weird to have GenericOne supported but not GenericZero since char does have a zero value.

@dsyme
Copy link
Contributor

dsyme commented Mar 23, 2020

It's mostly for consistency. It is weird to have GenericOne supported but not GenericZero since char does have a zero value.

Can you add the fix to ConstraintSolver.fs too, and also add test cases? Thanks

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Happypig375, I think this is a good change.

Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this contribution

@KevinRansom
Copy link
Contributor

@Happypig375 thank you for this contribution.

Kevin

@KevinRansom KevinRansom merged commit 1df8232 into dotnet:master May 28, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* Add char to types resolvable by GenericZero

* Add char to get_Zero solving

* Add char to test

* Add char to test

* Add char to test again

* Fix naming
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.

4 participants