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

Functorize Consistbl (with some background info on Compilation_unit.t) #2286

Merged
merged 2 commits into from Mar 7, 2019

Conversation

@mshinwell
Copy link
Contributor

commented Mar 5, 2019

In #2281 I mentioned a patch that provides a new version of Compilenv. One of the things this does, as part of the work to transition to new types for representing middle-end symbols, is to change the types that are used to represent the slightly tricky concept of "compilation unit names". In summary, the type Compilation_unit.t will be used as the identity of a compilation unit with OCaml semantics (as opposed to one fabricated for a startup file in Cmmgen, for example). Such an identity consists of the base name of the unit, of type Compilation_unit.Name.t, together with the -for-pack prefix (which may be empty). The type Compilation_unit.Name.t is very close to the type alias modname, although it is equipped with various operations, and does not currently occur earlier than the middle end.

Making the distinction between Compilation_unit.t and Compilation_unit.Name.t is a good, statically-enforced way of showing the distinction between places where we know the packing prefix of a compilation unit and places where we do not.

At present, in particular, -for-pack is not provided when compiling interfaces and thus there are various places, for example when dealing with interface dependencies in Asmlink and looking up symbols from global Ident.t values, where Compilation_unit.t values are not provided. Instead one has to start with a Compilation_unit.Name.t, and either work out or guess the -for-pack prefix (the latter in the case where the .cmx file is missing).

@lpw25 proposes to fix that particular nuance as part of his namespacing proposal, which would require that -for-pack is specified when building an .mli file. That aside, these subtleties are not obvious in the existing code, but are clear in the new version. This helps mere mortals such as myself understand what is going on.

To this end, with the advent of stronger types than string when dealing with dependencies in Asmlink and the like, it is necessary to have different versions of Consistbl. This necessity will probably persist even beyond any change to mandate -for-pack on .mli files because the type checker would quite possibly still use a different type from the middle end when indexing such table. This patch simply functorizes Consistbl to allow these multiple uses, along the way having the convenient side effect of replacing polymorphic equality by a more specialised version.

The diff is much smaller than it might appear; use a whitespace-aware diffing tool to review.

@mshinwell mshinwell force-pushed the mshinwell:consistbl_functor branch from 50ed613 to 0c51540 Mar 6, 2019

@Drup

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

I considered the question when I removed Tbl. I decided not to do that change and leave it when it was really needed, so it seems now is the time! I think this is a good idea (and I think it might be useful for the typechecker as well, if we decide to have a slightly better representation of compilation units).

It seems like you forgot some parts of the toplevel, but appart from that, the changes are fairly simple and straightforward, so as long as tests passes, I think it's fairly risk-free.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

@Drup Can you elaborate on "forgot some parts of the toplevel"? (This passes CI.)

@Drup

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Hmm, indeed it does, for some reason the version I was looking at didn't.

All good then!

@Drup

Drup approved these changes Mar 7, 2019

@mshinwell mshinwell force-pushed the mshinwell:consistbl_functor branch from 0c51540 to 3887e31 Mar 7, 2019

@mshinwell mshinwell merged commit d47ba6e into ocaml:trunk Mar 7, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.