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

Warning messages: fix references to extended descriptions #1554

Merged
merged 3 commits into from Jan 2, 2018

Conversation

Projects
None yet
3 participants
@Octachron
Copy link
Contributor

Octachron commented Jan 2, 2018

With the addition of the new polymorphism chapter, the references to the detailed warning explanations for warnings 4 and 57 had gone stale, this PR fixes this issue manually.

@trefis
Copy link
Contributor

trefis left a comment

I think you got one of the warning numbers wrong.

Changes Outdated
@@ -78,6 +78,9 @@ Working version
(Armaël Guéneau, with help and review from Florian Angeletti and Gabriel
Scherer)

- GPR#1554: warnings 4 and 57: fix reference to manual detailled explanation

This comment has been minimized.

@trefis

trefis Jan 2, 2018

Contributor

I think you meant 52 not 4.

This comment has been minimized.

@Octachron

Octachron Jan 2, 2018

Author Contributor

You are right, fixed.

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Jan 2, 2018

Also, I find it vaguely surprising that all the changes in the testsuite concern only warning 57. Do we have no tests for warning 52?

@Octachron Octachron force-pushed the Octachron:warning_messages_refs branch from 94dcc44 to 6570d99 Jan 2, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Jan 2, 2018

Do we have no tests for warning 52?

Indeed.

@trefis

This comment has been minimized.

Copy link
Contributor

trefis commented Jan 2, 2018

Is it wrong to consider that this PR is as good an occasion as any to add at least one test for warning 52? (testsuite/tests/warnings seems like the best place for it)

Changes Outdated
@@ -78,6 +78,9 @@ Working version
(Armaël Guéneau, with help and review from Florian Angeletti and Gabriel
Scherer)

- GPR#1554: warnings 52 and 57: fix reference to manual detailled explanation

This comment has been minimized.

@gasche

gasche Jan 2, 2018

Member

detailed

This comment has been minimized.

@Octachron

Octachron Jan 2, 2018

Author Contributor

Fixed, thanks.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Jan 2, 2018

Is it wrong to consider that this PR is as good an occasion as any to add at least one test for warning 52?

No, it is probably a good idea; I added a minimal test.

@trefis

trefis approved these changes Jan 2, 2018

Copy link
Contributor

trefis left a comment

LGTM.

Changes Outdated
@@ -78,6 +78,9 @@ Working version
(Armaël Guéneau, with help and review from Florian Angeletti and Gabriel
Scherer)

- GPR#1554: warnings 52 and 57: fix reference to manual detailed explanation
(Florian Angeletti, review by Thomas Refis)

This comment has been minimized.

@trefis

trefis Jan 2, 2018

Contributor

and Gabriel Scherer.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Jan 2, 2018

Heh.

For the record, I find it problematic that there are so many hardcoded variables here:

  • the chapter number may change
  • the Makefile has a list of the warnings that are documented

It would be nice to find a way, for both things, to either automate it or at least be able to check for consistency during the pregen step.

@Octachron Octachron force-pushed the Octachron:warning_messages_refs branch from f00f707 to b0ef01c Jan 2, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Jan 2, 2018

I agree that this fix is too brittle, and it would be nice to enforce some coherence.

For the first point, one possibility might be to make TeX export the chapter and section numbers of the warning reference section inside an utils/manualInfo.ml file and uses this information inside utils/warning.ml.

@gasche gasche merged commit 098705d into ocaml:trunk Jan 2, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member

gasche commented Jan 2, 2018

I don't like the idea of having the compiler codebase depend on the manual build -- at all, even if we play trick like committing the generated file. I think that an after-the-fact check would be better. If we follow your suggestion of having TeX export the chapter and section number, then we can have a manual-side script that contains some warning-raising ocaml code and compares the number with its own.

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.