Skip to content

Improve display of builtin types by the toplevel#13053

Merged
Octachron merged 3 commits into
ocaml:trunkfrom
samsa1:toplevel_printer
Apr 3, 2024
Merged

Improve display of builtin types by the toplevel#13053
Octachron merged 3 commits into
ocaml:trunkfrom
samsa1:toplevel_printer

Conversation

@samsa1
Copy link
Copy Markdown
Contributor

@samsa1 samsa1 commented Mar 27, 2024

Currently the toplevel uses type information to know how to display types such as lists.
However this works only if the type is exactly the builtin types and not an alias.

Thus the some examples are not displayed as nicely as they could :

# let f (l : 'a list) : 'a List.t = l
  ;;
val f : 'a list -> 'a List.t = <fun>
# f [3; 2];;
- : int List.t = List.(::) (3, [2])

The change proposed by this PR is to check if a type correspond to a builtin type through aliases to detect more occurrences of builtin types.

@samsa1
Copy link
Copy Markdown
Contributor Author

samsa1 commented Mar 27, 2024

Sorry I had a problem of mixed commits with another PR that is already merged. I'll try to clean up the diffs

Comment thread toplevel/genprintval.ml Outdated
@samsa1 samsa1 force-pushed the toplevel_printer branch from 90c620b to ee9c365 Compare March 27, 2024 18:27
@samsa1 samsa1 force-pushed the toplevel_printer branch from ee9c365 to efb49fa Compare March 27, 2024 18:28
@samsa1
Copy link
Copy Markdown
Contributor Author

samsa1 commented Mar 27, 2024

Ok, I managed to clean up the code, this should help reviewing

Copy link
Copy Markdown
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a clear improvement with a minimal code complexity cost. Even if not all possible cases are not handled, the more confusing one should be covered. In particular, using

open List

doesn't break the toplevel list printer anymore.

Thus I think it is better to merge this PR. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants