Split Import_info.t into Import_info.{Intf,Impl}.t
#1746
Closed
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
lukemaurer
requested review from
mshinwell,
xclerc,
lpw25,
antalsz,
ccasin,
goldfirere,
riaqn,
ncik-roberts,
stedolan,
lthls and
gretay-js
as code owners
This improves clarity, since every `Import.t` is statically either an interface import or an implementation import and they don't support quite the same operations. In particular, every implementation import (being an import of a .cmo/x file) must have a fully-specified compilation unit, whereas in implementation import might only have a name (with unknown pack prefix). Produced by reverting commit 4bd7544 from the 'instance-typing' branch.
lukemaurer
force-pushed
the
import-info-split
branch
from
d4b13aa to
a35803a
Compare
lukemaurer
added a commit
to lukemaurer/flambda-backend
that referenced
this pull request
Aug 16, 2023
To support parameterised libraries, it will help to sharpen the distinction
between `CU.Name.t` and `CU.t`. In particular, a `CU.Name.t` will mean the name
of a .cmi and a `CU.t` will mean the name of a .cmo/x.
Accordingly, the `cmi_name` field in `Cmi_format` changes from `CU.t` to
`CU.Name.t`. There is never a `CU.t` for a parameter module since it has no
.cmo/x, so for these, the `CU.t` is removed from the `Cmi_format` altogether. A
non-parameter .cmi still needs it, however, since we need to store the pack
prefix for the implementation .cmo/x. (We don't support pack prefixes for
parameter modules.) Accordingly, the `cmi_kind` field now has two variants:
```
type kind =
| Normal of { cmi_impl : CU.t }
| Parameter
```
As it happens, all this forces through a related change in `Import_info.t` so
that we can store import info for a parameter module, which has a CRC but no
compilation unit. Since the format of import info for interfaces and
implementations is diverging, a split API is introduced to `Import_info`. (This
is the same API that ocaml-flambda#1746 adds, but here it's optional and only used in the few
places that need it.)
lukemaurer
added a commit
to lukemaurer/flambda-backend
that referenced
this pull request
Aug 16, 2023
To support parameterised libraries, it will help to sharpen the distinction
between `Compilation_unit.Name.t` and `Compilation_unit.t`. In particular, a
`CU.Name.t` will mean the name of a .cmi and a `CU.t` will mean the name of a
.cmo/x.
Accordingly, the `cmi_name` field in `Cmi_format` changes from `CU.t` to
`CU.Name.t`. There is never a `CU.t` for a parameter module since it has no
.cmo/x, so for these, the `CU.t` is removed from the `Cmi_format` altogether. A
non-parameter .cmi still needs it, however, since we need to store the pack
prefix for the implementation .cmo/x. (We don't support pack prefixes for
parameter modules.) Accordingly, the `cmi_kind` field now has two variants:
```
type kind =
| Normal of { cmi_impl : CU.t }
| Parameter
```
As it happens, all this forces through a related change in `Import_info.t` so
that we can store import info for a parameter module, which has a CRC but no
compilation unit. Since the format of import info for interfaces and
implementations is diverging, a split API is introduced to `Import_info`. (This
is the same API that ocaml-flambda#1746 adds, but here it's optional and only used in the few
places that need it.)
|
I'll review this |
mshinwell
requested changes
Oct 16, 2023
There was a problem hiding this comment.
I think this would have been easier split into two PRs, and maybe that could still be done. The first could have all of the renames (adding .Impl or .Intf) and just two duplicate copies of the original Import_info module. Then the second PR could change the semantics of the two modules.
ocaml/utils/import_info.mli
Outdated
| @@ -12,7 +12,7 @@ | |||
| (* *) | |||
| (**************************************************************************) | |||
|
|
|||
| module CU = Compilation_unit | |||
| module CU := Compilation_unit | |||
|
|
|||
| (* CR mshinwell: maybe there should be a phantom type allowing to distinguish | |||
There was a problem hiding this comment.
This CR should be split and the first part removed (please change to CR xclerc:), since it's done (in a different way) by this patch.
| | Other of CU.Name.t * (CU.t * Digest.t) option | ||
| module Intf = struct | ||
| type t = | ||
| | Normal of CU.Name.t * Digest.t (* Unpacked, so compilation unit = name *) |
There was a problem hiding this comment.
Does this comment apply for all three cases? If so maybe clarify
There was a problem hiding this comment.
Adding comments to the other two
ocaml/utils/import_info.ml
Outdated
| match crc_with_unit with | ||
| | None -> Other (cu_name, None) | ||
| | Some (cu, crc) -> | ||
| (* For the moment be conservative and only use the [Normal] constructor |
There was a problem hiding this comment.
This comment seems out of date now, but maybe there should still be a comment here about the decision based on the pack prefix.
There was a problem hiding this comment.
Updating the comment.
|
|
||
| let create_normal cu ~crc = | ||
| match crc with Some crc -> Normal (cu, crc) | None -> Normal_no_crc cu | ||
| let impl t = |
There was a problem hiding this comment.
Can you clarify why the semantics of this was changed from that of the old cu function?
There was a problem hiding this comment.
Because an Import_info.Impl.t always has a full compilation unit, whereas it's normal for an Import_info.Intf.t not to have one.
I can split this off easily enough. I've pushed some review changes in the meantime. |
lukemaurer
added a commit
to lukemaurer/flambda-backend
that referenced
this pull request
Oct 16, 2023
PR ocaml-flambda#1746 will change the APIs and implementations to reflect the differences in the datatypes. (Currently, the major difference is that an `Impl.t` always has a full compilation unit, but parameterised libraries will add more differences.)
lukemaurer
added a commit
to lukemaurer/flambda-backend
that referenced
this pull request
Oct 16, 2023
Done: #1933 just copies and pastes |
lukemaurer
changed the base branch from
main
to
lmaurer/import-info-split-copy
lukemaurer
added a commit
to lukemaurer/flambda-backend
that referenced
this pull request
Apr 30, 2024
[Import_info.t] is now split between the intf and impl cases, which will make it easier for a subsequent PR (which will likely supersede ocaml-flambda#1746 and ocaml-flambda#1933) to pull apart the types and force all callers to choose. Also added a [Consistbl]-centric API for each of [Intf] and [Impl] to make it easy again to convert between import info and an entry in a [Consistbl].
|
Superseded by #2552. |
lpw25
pushed a commit
that referenced
this pull request
May 15, 2024
…1753) To support parameterised libraries, it will help to sharpen the distinction between `Compilation_unit.Name.t` and `Compilation_unit.t`. In particular, a `CU.Name.t` will mean the name of a .cmi and a `CU.t` will mean the name of a .cmo/x. Accordingly, the `cmi_name` field in `Cmi_format` changes from `CU.t` to `CU.Name.t`. There is never a `CU.t` for a parameter module since it has no .cmo/x, so for these, the `CU.t` is removed from the `Cmi_format` altogether. A non-parameter .cmi still needs it, however, since we need to store the pack prefix for the implementation .cmo/x. (We don't support pack prefixes for parameter modules.) Accordingly, the `cmi_kind` field now has two variants: ``` type kind = | Normal of { cmi_impl : CU.t } | Parameter ``` As it happens, all this forces through a related change in `Import_info.t` so that we can store import info for a parameter module, which has a CRC but no compilation unit. Since the format of import info for interfaces and implementations is diverging, a split API is introduced to `Import_info`. (This is the same API that #1746 adds, but here it's optional and only used in the few places that need it.)
lukemaurer
added a commit
to lukemaurer/flambda-backend
that referenced
this pull request
Oct 23, 2024
…o for parameters (ocaml-flambda#1753) To support parameterised libraries, it will help to sharpen the distinction between `Compilation_unit.Name.t` and `Compilation_unit.t`. In particular, a `CU.Name.t` will mean the name of a .cmi and a `CU.t` will mean the name of a .cmo/x. Accordingly, the `cmi_name` field in `Cmi_format` changes from `CU.t` to `CU.Name.t`. There is never a `CU.t` for a parameter module since it has no .cmo/x, so for these, the `CU.t` is removed from the `Cmi_format` altogether. A non-parameter .cmi still needs it, however, since we need to store the pack prefix for the implementation .cmo/x. (We don't support pack prefixes for parameter modules.) Accordingly, the `cmi_kind` field now has two variants: ``` type kind = | Normal of { cmi_impl : CU.t } | Parameter ``` As it happens, all this forces through a related change in `Import_info.t` so that we can store import info for a parameter module, which has a CRC but no compilation unit. Since the format of import info for interfaces and implementations is diverging, a split API is introduced to `Import_info`. (This is the same API that ocaml-flambda#1746 adds, but here it's optional and only used in the few places that need it.)
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.
Depends on #1933.
This improves clarity, since every
Import.tis statically either an interface import or an implementation import and they don't support quite the same operations. In particular, every implementation import (being an import of a .cmo/x file) must have a fully-specified compilation unit, whereas an interface import might only have a name (with unknown pack prefix).Produced by reverting commit 4bd7544 from the 'instance-typing' branch.