Readability improvements around instance_constructor #10987
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Ping @gasche (as he invited me to ping him explicitely). |
gasche
reviewed
Feb 15, 2022
| let (args, cstr_res, _ex) = Ctype.instance_constructor cdescr in | ||
| let (args, cstr_res, _ex) = | ||
| Ctype.instance_constructor Keep_existentials_flexible cdescr | ||
| in |
There was a problem hiding this comment.
Out of curiosity, do we know what happens here? _ex is ignored, so we cannot have existentials at this point? Or can there, and keeping existentials flexible is the right thing? (I guess we re-generalize later?)
There was a problem hiding this comment.
I'm not sure I understand you're question... but keeping existential variables flexible here does seem like the right thing yes.
gasche
approved these changes
Mar 4, 2022
|
The CI passed, so I merged. Thanks for the review @gasche ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sorry for the highly controversial PR which is neither related to multicore, nor fixing any bug.
As I said earlier, I was a bit confused (I'm easily confused) when trying to review #10959. The parts which confused me are actually related to #9584.
Anyway, I'm proposing 3 commits which shouldn't change at all the observable behavior of the compiler, but which make things slightly more readable in my opinion:
instance_constructorwithout being careful, it seemed like we were sometimes (whenin_patternisSome) turning existentials into local types, then taking a different instance of the existentials, and returning that, ignoring the abstract types.That's of course not what happens: everything lives in the same "copy scope", so the second instance actually encounter a
Tsubstnodes and gets the local type from before. It was a bit too subtle for me though. So I changed the code to only take one obvious instance of the existentials.(That's of course not quite true: another instance will be taken when getting a copy of
ty_args... so I'll admit that it's debatable that this commit actually makes things more readable)in_patternargument toinstance_constructor. Now it's to be typing a pattern where the existentials haven't been explicitely named, so I feel justified in changingin_patternfor something indicating which behavior to adopt, rather than which context we were being called from a few versions of OCaml ago.Please forgive me.