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

type return/break/continue expressions with monomorphs instead of Dynamic (see #10744) #10745

Merged
merged 5 commits into from
Aug 3, 2022

Conversation

nadako
Copy link
Member

@nadako nadako commented Jul 1, 2022

this is what we already do for throw since 2008 (9cee2fc)

@nadako nadako requested a review from Simn July 1, 2022 12:29
@nadako
Copy link
Member Author

nadako commented Jul 1, 2022

I'm wondering if we should be a bit more clever and type all terminators as Void if they are used in WithType.NoValue context. Less monomorphs is always better, and we could save some memory with this, I guess.

(ideally we of course want a bottom type but that's a different topic)

@nadako
Copy link
Member Author

nadako commented Jul 1, 2022

I'm wondering if we should be a bit more clever and type all terminators as Void if they are used in WithType.NoValue context.

Tried that quickly and ran into problems with inlined returns, so it needs more investigation.

@Simn
Copy link
Member

Simn commented Jul 4, 2022

@ncannasse You wouldn't happen to remember why this was typed as Dynamic? I recall discussing this with you like 10 years ago and it made sense to me at the time, but now I'm not really seeing why this should be the case.

@Simn
Copy link
Member

Simn commented Aug 2, 2022

Looking at this again, I wonder if we should only create monomorphs for this when we're typing a value. I don't see what we gain by creating monos for any block-level return expression and such. I'm actually trying to have less wild monos floating around, so this would work a bit against that.

What do you think?

@nadako
Copy link
Member Author

nadako commented Aug 2, 2022

Makes sense. I think I've tried that but had some issue with inlining and didn't have time to figure it out since then...

@@ -36,6 +36,12 @@ open Operators
(* ---------------------------------------------------------------------- *)
(* TOOLS *)

let mono_or_dynamic ctx with_type p = match with_type with
| WithType.NoValue ->
t_dynamic
Copy link
Member Author

Choose a reason for hiding this comment

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

should it be Void maybe?

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit worried about some crazy inline situation where we end up with Void in a value-place... With Dynamic we at least have the same behavior as before.

@Simn Simn merged commit 02b4357 into HaxeFoundation:development Aug 3, 2022
@nadako nadako deleted the 10744 branch August 3, 2022 12:03
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