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

allow syntax Functor(val x) #949

Merged
merged 2 commits into from Dec 13, 2016

Conversation

Projects
None yet
5 participants
@sliquister
Contributor

sliquister commented Dec 5, 2016

Instead of just Functor((val x)) right now.

@garrigue

This comment has been minimized.

Show comment
Hide comment
@garrigue

garrigue Dec 6, 2016

Contributor

Makes sense.

Contributor

garrigue commented Dec 6, 2016

Makes sense.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Dec 6, 2016

Contributor

Can you update the manual accordingly (in exten.tex)?

Contributor

alainfrisch commented Dec 6, 2016

Can you update the manual accordingly (in exten.tex)?

@sliquister

This comment has been minimized.

Show comment
Hide comment
@sliquister

sliquister Dec 7, 2016

Contributor

I don't know if you want me to note in the manual something like "starting at 4.05 we can do F(val x)", or change the grammar in the manual. In the latter case, it's easy to change the manual the same way the parser was changed, but it sounds like too much detail. The grammar is already approximate, for instance:

pattern: ...
         | 'lazy' pattern

but lazy lazy _ is not valid pattern.

Contributor

sliquister commented Dec 7, 2016

I don't know if you want me to note in the manual something like "starting at 4.05 we can do F(val x)", or change the grammar in the manual. In the latter case, it's easy to change the manual the same way the parser was changed, but it sounds like too much detail. The grammar is already approximate, for instance:

pattern: ...
         | 'lazy' pattern

but lazy lazy _ is not valid pattern.

@mauny

This comment has been minimized.

Show comment
Hide comment
@mauny

mauny Dec 7, 2016

lazy (lazy _) is a valid pattern, but needs parentheses.

mauny commented Dec 7, 2016

lazy (lazy _) is a valid pattern, but needs parentheses.

@Octachron

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Dec 7, 2016

Contributor

I don't think that updating the grammar here would be very useful since the grammar description in this chapter is indeed quite approximate. I think that adding an "introduced in 4.05" note in the header and a short mention in the "Conversely, the module expression ( val expr : package-type )…" paragraph should be enough. Eventually, an example exercising the new syntax might be nice: it would help people that skim over the text description and focus on code examples.

Contributor

Octachron commented Dec 7, 2016

I don't think that updating the grammar here would be very useful since the grammar description in this chapter is indeed quite approximate. I think that adding an "introduced in 4.05" note in the header and a short mention in the "Conversely, the module expression ( val expr : package-type )…" paragraph should be enough. Eventually, an example exercising the new syntax might be nice: it would help people that skim over the text description and focus on code examples.

@sliquister

This comment has been minimized.

Show comment
Hide comment
@sliquister

sliquister Dec 9, 2016

Contributor

I added a change to the manual, but I don't think it's a good idea because the language is not changing here.

I'll point out that the manual doesn't build without commenting out the check-stdlib-modules check that complains about spacetime not being documented.

Contributor

sliquister commented Dec 9, 2016

I added a change to the manual, but I don't think it's a good idea because the language is not changing here.

I'll point out that the manual doesn't build without commenting out the check-stdlib-modules check that complains about spacetime not being documented.

@Octachron

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Dec 9, 2016

Contributor

I don't think it's a good idea because the language is not changing here

I am not sure I follow your point here? With this PR, there are some code fragments that will parse with 4.05 but not with 4.04. This is a visible change for users that ought to be documented.

I'll point out that the manual doesn't build without commenting out the check-stdlib-modules check that complains about spacetime not being documented.

This is fixed in 4.04 but the commit fixing this problem has not yet been merged in trunk.

Contributor

Octachron commented Dec 9, 2016

I don't think it's a good idea because the language is not changing here

I am not sure I follow your point here? With this PR, there are some code fragments that will parse with 4.05 but not with 4.04. This is a visible change for users that ought to be documented.

I'll point out that the manual doesn't build without commenting out the check-stdlib-modules check that complains about spacetime not being documented.

This is fixed in 4.04 but the commit fixing this problem has not yet been merged in trunk.

@sliquister

This comment has been minimized.

Show comment
Hide comment
@sliquister

sliquister Dec 10, 2016

Contributor

The change is documented in the changelog, just like any random typer or backend bug that prevents valid programs from being successfully compiled.

Contributor

sliquister commented Dec 10, 2016

The change is documented in the changelog, just like any random typer or backend bug that prevents valid programs from being successfully compiled.

@garrigue

This comment has been minimized.

Show comment
Hide comment
@garrigue

garrigue Dec 13, 2016

Contributor

Travis is successful but Appveyor fails.
Is it safe to merge?

Contributor

garrigue commented Dec 13, 2016

Travis is successful but Appveyor fails.
Is it safe to merge?

@sliquister

This comment has been minimized.

Show comment
Hide comment
@sliquister

sliquister Dec 13, 2016

Contributor

The Appveyor failure certainly looks spurious: the tests pass locally, and Appveyor is ok with the first commit, and the second commit only adds a couple of sentences to the manual so it's hard to see how would break dozens of tests.

Contributor

sliquister commented Dec 13, 2016

The Appveyor failure certainly looks spurious: the tests pass locally, and Appveyor is ok with the first commit, and the second commit only adds a couple of sentences to the manual so it's hard to see how would break dozens of tests.

@garrigue

This comment has been minimized.

Show comment
Hide comment
@garrigue

garrigue Dec 13, 2016

Contributor

OK, then I merge. Thank you for the PR.

Contributor

garrigue commented Dec 13, 2016

OK, then I merge. Thank you for the PR.

@garrigue garrigue merged commit d8fc8dc into ocaml:trunk Dec 13, 2016

1 of 2 checks passed

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

@sliquister sliquister deleted the sliquister:no-double-parens branch Dec 13, 2016

@sliquister

This comment has been minimized.

Show comment
Hide comment
@sliquister

sliquister Dec 14, 2016

Contributor

Thanks.

Contributor

sliquister commented Dec 14, 2016

Thanks.

@dra27 dra27 referenced this pull request Dec 14, 2016

Merged

Rebase 4.04.0 #962

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

MPR#7216: allow syntax Functor(val x) (#949)
* allow syntax Functor(val x)
* manual: mention the first-class module tweak
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment