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

Fix MPR#7751 #1657

Merged
merged 6 commits into from Mar 27, 2018

Conversation

Projects
None yet
5 participants
@garrigue
Copy link
Contributor

garrigue commented Mar 13, 2018

Fixes MPR#7751: The toplevel prints some concrete types as abstract.

This was a side effect of the fix of MPR#7734, which prevented most printing functions from loading cmis, whereas this should only be done for errors.

This PR adds and error parameter to Printtyp.wrap_printing_env, and disables the loading of cmis only when it is set to true.

@garrigue

This comment has been minimized.

Copy link
Contributor Author

garrigue commented Mar 13, 2018

There is no test, because the original report relies on loading compilerlibs from the toplevel, which is hard to do in a test, and would be fragile (the printing is going to change).
If somebody volunteers a small test, I buy.

@garrigue garrigue requested a review from gasche Mar 13, 2018

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Mar 13, 2018

@matejkosik

This comment has been minimized.

Copy link

matejkosik commented Mar 13, 2018

Thank you.

I have compiled this branch. I haven't studied the source code changes, but the behavior of Ocaml toplevel seems to be the one that make sense. No more <abstr> in place of values belonging to transparent types.

@garrigue

This comment has been minimized.

Copy link
Contributor Author

garrigue commented Mar 22, 2018

@shindere Have a look at the test I added.
I needed 12 dots to go back to the top directory, and the test cannot be run through make one as a result. If you have a way to improve that, I buy it.
On approach could be to set the standard library directory to the top of the hierarchy.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Mar 22, 2018

@garrigue garrigue force-pushed the garrigue:fix_mpr7751 branch from e6a9c5b to 4d05b65 Mar 22, 2018

@garrigue

This comment has been minimized.

Copy link
Contributor Author

garrigue commented Mar 22, 2018

Great! That did it.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Mar 22, 2018

@garrigue

This comment has been minimized.

Copy link
Contributor Author

garrigue commented Mar 26, 2018

I’ll merge this soon, since this is a pain to keep tabs on PRs waiting for a review.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Mar 26, 2018

@trefis

trefis approved these changes Mar 26, 2018

Copy link
Contributor

trefis left a comment

The idea behind the change is fine and an improvement over the current state of things.

Given that the new parameter to wrap_printing_env is not optional I assume all the call sites have been considered and do the proper thing.

Feel free to merge whenever.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Mar 26, 2018

(I actually just realized that I had a review assigned to me, sorry @garrigue! I trust @trefis (or Frédéric) to do a better review than I would, given that they have looked at both type printing and .cmi loading over the course of their Merlin work, and also I have almost no time for reviews right now.)

@garrigue

This comment has been minimized.

Copy link
Contributor Author

garrigue commented Mar 27, 2018

Thank you all. I'll merge now.

@garrigue garrigue merged commit 3d33bd4 into ocaml:trunk Mar 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@trefis trefis referenced this pull request Jul 16, 2018

Merged

Fix 7821 #1908

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.