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

Manual: add extended description of warning 57 #468

Closed
wants to merge 2 commits into from

Conversation

Octachron
Copy link
Member

This PR extracts the detailed comment on warning 57 in typing/parmatch.ml, rewords it slightly
to be more neutral, and adds it to a new section in the manual, just after the section 8.4 on common compiler errors.
Ideally, this new section would give a place to describe in detail future or past warnings and the rationale behind them.

@gasche
Copy link
Member

gasche commented Feb 12, 2016

Thanks! I would like to contribute to this new section as well. I may create an upstream branch to coordinate our work, and then ask for a merge in trunk (or the release branch) when we have a few more warnings documented. (Of course the goal is not to cover everything, but just one would feel odd, wouldn't it?)

match the scrutinee against the pattern, if it matches, test the guard,
and if the guard passes, take the branch.
In particular, consider the input "(Const"~\var{a}", Const"~\var{b}")", where
\var{a} fails the test "is_neutral"~\var{f}, while \var{b} passes the test
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this say "" fails the test "is_neutral"~\var{a} "" instead of var{f} ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. Fixed.

@Octachron
Copy link
Member Author

Adding more warnings to the section would be certainly better. If you prefer to have an upstream branch to collect them, it is perfectly fine with me.

@gasche
Copy link
Member

gasche commented Feb 12, 2016

So I just created a new branch warnings-documentation. Would you resend your PR there?

(You need to be prepared to have one more Changes line in your name once this gets merged upstream :-)

@Octachron
Copy link
Member Author

PR resend.

@gasche
Copy link
Member

gasche commented Feb 12, 2016

cc @damiendoligez : you may have an opinion about this (low priority) stuff.

@damiendoligez
Copy link
Member

More documentation is always better, but I'm starting to get uncomfortable with the length of the lines in some of the new warning messages.

@Octachron
Copy link
Member Author

To reduce a little the warning length, the reference (see manual section 8.5) could be replaced by (manual 8.5).

A more long term solution might be to use Format for printing warnings and add a break hint after Ambiguous guarded patter,.

Or maybe, just hard code a line break here.

Keryan-dev added a commit to Keryan-dev/ocaml that referenced this pull request Jun 15, 2021
Gbury pushed a commit to Gbury/ocaml that referenced this pull request Jun 22, 2021
chambart pushed a commit to chambart/ocaml-1 that referenced this pull request Feb 1, 2022
* Do not run the offset check by default

* Better style for the comment

* Add comment about to_cmm handling of missing offset
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
- Ocamlorg_package.Documentation.item is now a polymorphic variant, to ease
  rendering the items in eml
- Add breadcrumbs.eml as a component rendering navigation breadcrumbs
- Update handler.ml, package_documentation.eml and package_layout.eml to
  render the breadcrumbs

Co-authored-by: Thibaut Mattio <thibaut.mattio@gmail.com>
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.

None yet

4 participants