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

always associate a scope to a type (previously it was optional) #2072

Merged
merged 3 commits into from Oct 5, 2018

Conversation

Projects
None yet
3 participants
@trefis
Copy link
Contributor

commented Sep 26, 2018

Previously, not having a scope meant the type was used in every context, now we set the scope to Btype.lowest_level to mean the same thing.
The equivalence was made obvious by the recent changes to identifiers scoping.

This should reduce the size of .cmi files.

@trefis trefis force-pushed the trefis:always-scope branch from aea8465 to c080505 Sep 26, 2018

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2018

Could you be a bit clearer: when was the level None?
My understanding was that an explicit level was needed for all local type definitions, so this would only be for type definitions stored in a cmi?
The space improvement should be extremely small, so I think this rather about code simplification.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

You are correct: it's mostly about simplification, but I thought we'd regain some space in cmis, I was wrong.
An explicit scope (i.e. the Some case) was set only for local types (newtypes and fresh existential types), everything else (which covers what ends up in cmi files) was None.

Still, I think the change is worth having.

trefis added some commits Sep 26, 2018

always associate a scope to a type (previously it was optional)
Previously, not having a scope meant the type was used in every context,
now we set the scope to "Btype.lowest_level" to mean the same thing.
The equivalence was made obvious by the recent changes to identifiers
scoping.

@trefis trefis force-pushed the trefis:always-scope branch from c080505 to 25d5248 Oct 5, 2018

@lpw25

lpw25 approved these changes Oct 5, 2018

Copy link
Contributor

left a comment

Looks correct and an improvement.

@trefis trefis merged commit 8d2dc0a into ocaml:trunk Oct 5, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@trefis trefis deleted the trefis:always-scope branch Oct 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.