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 mpr7620 #1317

Merged
merged 4 commits into from Sep 12, 2017

Conversation

Projects
None yet
3 participants
@garrigue
Copy link
Contributor

commented Sep 5, 2017

Call Typecore.force_delayed_checks before printing the signature for ocamlc -i

Cf MPR#7620.

garrigue added some commits Sep 5, 2017

@garrigue garrigue requested a review from alainfrisch Sep 6, 2017

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2017

Cf the comment a bit below:

It is important to run these checks after the inclusion test above, so that value declarations which are not used internally but exported so that value declarations which are not used internally but exported are not reported as being unused.

I think that running the checks without checking inclusion with the .mli signature would result in false positives.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2017

Sorry, I did not see your comments on Mantis. But this PR currently does nothing to disable the unused warnings in case of -i, right?

But if we consider that ocamlc -i is supposed to run the full type-checker and report all warnings as the compiler would normally do, why no perform the inclusion check as well?

@garrigue

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2017

I checked, but it seems that these warnings are not enabled by default (am I wrong?), so there was nothing to do.

Sometimes one calls -i to generate a new interface, so coercing with the old one would be self-defeating.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2017

I checked, but it seems that these warnings are not enabled by default (am I wrong?), so there was nothing to do.

But they could be enabled explicitly. I don't know why people would do that, but it seems better to explicitly disable the warnings (or just not register the corresponding delayed checks).

Anyway, this is rather minor, and the current patch is already better than the status quo, so I think this can go in 4.06.

@alainfrisch
Copy link
Contributor

left a comment

LGTM. As said, it would be better to protect against the unused warnings in case they are explicitly enabled, but this is rather minor.

@garrigue garrigue merged commit e286dbd into ocaml:trunk Sep 12, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2017

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.