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.

Sign up for GitHub

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

Jump to bottom

Reduce isAppTy calls in TypeDefinitelyHasEquality #8286

Merged
merged 1 commit into from
Jan 21, 2020
Merged

Reduce isAppTy calls in TypeDefinitelyHasEquality #8286

merged 1 commit into from
Jan 21, 2020

Conversation

forki
Copy link
Contributor

@forki forki commented Jan 19, 2020

No description provided.

@smoothdeveloper
Copy link
Contributor

smoothdeveloper commented Jan 19, 2020 •
edited
Loading

Do we know stripTyEqns can turn some TType which is a TType_app into another one?

If this is known to be false, isAppTy could be simplified to skip call to stripTyEqns altogether, which should be much faster.

The opposite (a not TType_app turned into one through stripTyEqns) is know to be false already.

@smoothdeveloper
Copy link
Contributor

TType_app can turn into something else in stripTyEqns so can't skip that call.

cartermp
cartermp approved these changes Jan 20, 2020
View reviewed changes
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.

Minor nit but I like the change

src/fsharp/AugmentWithHashCompare.fs
// Check the (possibly inferred) structural dependencies
(tinst, tcref.TyparsNoRange)
||> List.lengthsEqAndForall2 (fun ty tp -> not tp.EqualityConditionalOn || TypeDefinitelyHasEquality g ty)
| _ -> false
Copy link
Contributor

@cartermp cartermp Jan 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: the _ case (as mentioned in another PR) feels less appropriate since ValueNone is the only other case here. The above usage is fine though since the ValueSome case prior also has a when clause.

@KevinRansom KevinRansom merged commit bb46e37 into dotnet:master Jan 21, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
@forki @nosami
Reduce isAppTy calls in TypeDefinitelyHasEquality (dotnet#8286)
42db3bf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cartermp cartermp cartermp approved these changes

@KevinRansom KevinRansom KevinRansom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@forki @smoothdeveloper @KevinRansom @cartermp