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

Instances for empty type. #393

Merged
merged 5 commits into from Oct 25, 2019
Merged

Instances for empty type. #393

merged 5 commits into from Oct 25, 2019

Conversation

arthuraa
Copy link
Contributor

Motivation for this change

Several users have expressed their wish for an empty type in mathcomp (e.g. #225). This PR adds a void notation for the Empty_set type of the standard library, along with instances of basic classes.

Things done/to do
  • added corresponding entries in CHANGELOG_UNRELEASED.md
  • added corresponding documentation in the headers

The documentation already existed in eqtype. I have added a note in fintype, but left choice alone since the instance list there was not exhaustive.

The notation is defined in ssrfun. Since most of this file lives in the Coq source now, I did not add a corresponding entry there. Would it make sense to do so?

Automatic note to reviewers

Read this Checklist and make sure there is a milestone.

Copy link
Member

@CohenCyril CohenCyril left a comment

Choose a reason for hiding this comment

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

LGTM

@amahboubi amahboubi self-assigned this Oct 21, 2019
@amahboubi
Copy link
Member

Hi @arthuraa, thanks for your contribution! Regarding your question:

The notation is defined in ssrfun. Since most of this file lives in the Coq source now, I did not add a corresponding entry there. Would it make sense to do so?

I think you should add a header to mathcomp's ssrfun, since you introduced the notation here. This is what happens, e.g., in file ssreflect.v. I will merge as soon as you add this documentation.

@arthuraa
Copy link
Contributor Author

@amahboubi Sounds good! How does this look?

@amahboubi
Copy link
Member

It looks great.

@CohenCyril
Copy link
Member

Oh, but if you add something to the local ssrfun you should also add it to Coq's... did you? If not, could you do it?

@arthuraa
Copy link
Contributor Author

@CohenCyril Just did: coq/coq#10932.

@amahboubi
Copy link
Member

@CohenCyril, the PR has been merged in Coq. Anything else you would like @arthuraa to do before we merge?

Copy link
Member

@CohenCyril CohenCyril left a comment

Choose a reason for hiding this comment

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

@amahboubi nothing more, LGTM, we can merge

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

3 participants