OxCaml: Support for modalities#1420
Conversation
art-w
force-pushed
the
oxcaml-modalities
branch
3 times, most recently
from
e6ed225 to
a1f6bca
Compare
panglesd
left a comment
panglesd
left a comment
There was a problem hiding this comment.
Looks good! I just want confirmation on:
- not loading modalities on cmt files
- the discrepancy between the input of the test and the output (see the comment there)
The other comments are less important.
| let type_ = Cmi.read_type_expr env pat.pat_type in | ||
| let value = Abstract in | ||
| [Value {id; source_loc; doc; type_; value}] | ||
| [Value {id; source_loc; doc; type_; value; modalities = []}] |
There was a problem hiding this comment.
Are modalities ignored on cmt loading on purpose?
| (match mty.mty_type with | ||
| #if defined OXCAML | ||
| | Mty_signature sg -> | ||
| (* For modules with modalities (e.g. [module M : S @@ portable]), |
There was a problem hiding this comment.
Is it what the docs describes as (they use the "modes" word but it is also in the "modalities" section):
Support for modules with modes is being worked on and not ready for wide adoption. More documentation will come as it becomes ready.
If so, I'm fine with the current code being merged, but I would need to update/re-review when there are more documentation (eg, can we add modalities on functor module types?)
|
|
||
| val read_modalities : | ||
| Types.mutability -> | ||
| Mode.Modality.Const.t -> | ||
| Odoc_model.Lang.Modalities.t | ||
|
|
||
| val read_value_modalities : | ||
| Mode.Modality.t -> | ||
| Odoc_model.Lang.Modalities.t |
There was a problem hiding this comment.
I think we would avoid quite a lot of #if defined OXCAML if we had read_value_modalities, read_field_modalities, etc., taking not Mode.Modality.t but the ast nodes above.
The "non-oxcaml" version of these function would exist and return [].
(This is nitpicky! But I tend to try to avoid having cppo in too many places, I find it more readable and I can ocamlformat individual structure items. Feel free to ignore if you want, it is really good-enough!)
| | D of int @@ portable * string @@ global | ||
| (** Per-element modalities in a constructor tuple. *) |
There was a problem hiding this comment.
For completeness it could be nice to add an inline record D' of {a : int @@ portable; b : string @@ global }
| f_unique : opaque @@ unique; | ||
| (** Uniqueness modality. *) | ||
| f_aliased : opaque @@ aliased; | ||
| (** Uniqueness modality (identity, not rendered). *) |
There was a problem hiding this comment.
In my test, that is the other way around: unique is not rendered and aliased is rendered. What is to blame? The comment or the output of the test?
|
|
||
| (** {1 Modalities on constructor arguments} *) | ||
|
|
||
| type modalities_cstr = |
There was a problem hiding this comment.
I think it would be worth adding a GADT to the test!
| (** No modality, for reference. *) | ||
| } | ||
|
|
||
| (** {1 Multiple modalities on a field} *) |
There was a problem hiding this comment.
It would be nice if the subheadings were of level 2 (this file keeps growing and it would improve navigation in the browser!)
| module M3 : sig @@ contended | ||
| val f : string -> bool | ||
| type s | ||
| end | ||
| (** [contended] modality applied to all definitions in the module. *) |
There was a problem hiding this comment.
Maybe worth adding val that also sets some modalities (on the contendedness axis and/or on another axis)
2 participants
Builds on top of #1410, only the last two commits are new (one commit to test, one to fix)
This PR adds support for OxCaml modalities, fixes #1416