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

[C++] Support requires-clause in more places #10286

Closed
wants to merge 4 commits into from
Closed

Conversation

jbms
Copy link
Contributor

@jbms jbms commented Mar 22, 2022

Feature or Bugfix

  • Feature

Purpose

Previously a C++20 requires-clause was only supported on function
declarations. However, the C++ standard allows a require-clause on
class/union templates, alias templates, and variable templates, and
also allows a requires clause after each template parameter list, not
just the final one.

This moves the requiresClause to be a property of ASTTemplateParams
rather than ASTDeclaration to better match the C++ grammar and
allows requires clauses in many places that are supported by C++20 but
were not previously allowed by Sphinx, namely:

  • On class/union templates, alias templates, and variable templates

  • After each template parameter list, not just the last one.

  • After the template parameter list in template template parameters.

Additionally:

  • This adds support for template parameters on unions.

  • This adds support for trailing requires clauses on functions without
    a template prefix. This is allowed by C++20 for non-template members
    of class templates.

When encoding the id, the requires clause of the last template
parameter list is treated specially in order to preserve compatibility
with existing v4 ids.

@jakobandersen jakobandersen self-assigned this Mar 23, 2022
@jakobandersen jakobandersen added type:enhancement enhance or introduce a new feature domains:cpp labels Mar 23, 2022
@jakobandersen jakobandersen added this to the 4.5.0 milestone Mar 23, 2022
@jbms jbms force-pushed the 4.x branch 3 times, most recently from b41aa75 to c1dce07 Compare March 23, 2022 23:13
@jbms
Copy link
Contributor Author

jbms commented Mar 23, 2022

Note: I made some major changes to this PR to support a lot of additional things.

As it is currently implemented, it does not prohibit requires clauses on concepts even though those are disallowed by C++. But I don't think that is a problem.

Note: I noticed that there is support for non-standard concept introduction syntax, which I guess was part of an earlier concepts proposal. Should that be removed since it is not supported by C++?

@tk0miya tk0miya modified the milestones: 4.5.0, 5.0.0 Mar 27, 2022
Copy link
Contributor

@jakobandersen jakobandersen left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. There are a number of structural changes to the code, and you list multiple changes. Please split it up into multiple commits to make it easier to review.

sphinx/domains/cpp.py Outdated Show resolved Hide resolved
sphinx/domains/cpp.py Show resolved Hide resolved
@jakobandersen
Copy link
Contributor

As it is currently implemented, it does not prohibit requires clauses on concepts even though those are disallowed by C++. But I don't think that is a problem.

It would be nice to not allow it, unless it is really cumbersome to disallow.

Note: I noticed that there is support for non-standard concept introduction syntax, which I guess was part of an earlier concepts proposal. Should that be removed since it is not supported by C++?

They were indeed implemented during the Concepts TS time-frame. If they are in the way of implementing new things we should deprecate them, otherwise I see no harm in letting them be.

@jbms jbms changed the title [C++] Support requires-clause on type, class, 'member' declarations [C++] Support requires-clause in more places Mar 27, 2022
@jbms
Copy link
Contributor Author

jbms commented Mar 27, 2022

Thanks for reviewing this change. I have split it up into multiple commits, and added a check to prevent a requires-clause on concepts.

@jbms jbms requested a review from jakobandersen March 27, 2022 18:28
Previously a C++20 requires-clause was only supported on `function`
declarations.  However, the C++ standard allows a require-clause on
class/union templates, alias templates, and variable templates, and
also allows a requires clause after each template parameter list, not
just the final one.

This moves the requiresClause to be a property of `ASTTemplateParams`
rather than `ASTDeclaration` to better match the C++ grammar and
allows requires clauses in many places that are supported by C++20 but
were not previously allowed by Sphinx, namely:

- On class templates, alias templates, and variable templates

- After each template parameter list, not just the last one.

- After the template parameter list in template template parameters.

When encoding the id, the requires clause of the last template
parameter list is treated specially in order to preserve compatibility
with existing v4 ids.
@jbms
Copy link
Contributor Author

jbms commented Apr 7, 2022

@jakobandersen Please let me know if there is anything I can do to help make progress with this and my other C++ PRs. Thanks!

@tk0miya tk0miya changed the base branch from 4.x to 5.x May 22, 2022 12:53
@tk0miya tk0miya changed the base branch from 5.x to 5.0.x May 22, 2022 12:54
@tk0miya tk0miya modified the milestones: 5.0.0, 5.1.0 May 29, 2022
@AA-Turner AA-Turner changed the base branch from 5.0.x to 5.x June 27, 2022 21:26
@jakobandersen
Copy link
Contributor

When encoding the id, the requires clause of the last template
parameter list is treated specially in order to preserve compatibility
with existing v4 ids.

Is this really needed? As currently only the clause being ID-encoded is allowed, then all currently allowed declarations will have the same ID afterwards anyway. Or am I missing something?

@jbms
Copy link
Contributor Author

jbms commented Jun 28, 2022

When encoding the id, the requires clause of the last template
parameter list is treated specially in order to preserve compatibility
with existing v4 ids.

Is this really needed? As currently only the clause being ID-encoded is allowed, then all currently allowed declarations will have the same ID afterwards anyway. Or am I missing something?

Previously, the requires clause was encoded after the template parameter list, so with a requires clause after the last template parameter list we would have:

"I" + template_parameter_list_id + "E" + "IQ" + requires_clause_id + "E"

if we didn't special case requires clauses on the last template parameter list, it would instead be:

"I" + template_parameter_list_id + "IQ" + requires_clause_id + "E" + "E"

@jbms
Copy link
Contributor Author

jbms commented Jun 28, 2022

I didn't see anything in the Itanium ABI regarding requires clauses --- not sure if you just invented a convention for them or if you used a convention defined elsewhere. With this change, I just used a convention that seemed to make sense.

@AA-Turner
Copy link
Member

@jakobandersen Should we move this to 5.x, or is it nearly ready to be merged for 5.1?

A

@AA-Turner AA-Turner mentioned this pull request Jul 18, 2022
@jakobandersen
Copy link
Contributor

@jakobandersen Should we move this to 5.x, or is it nearly ready to be merged for 5.1?

I won't have time to properly dig back into it before the weekend, so please go ahead with the release and move this to 5.2.

@jakobandersen
Copy link
Contributor

if we didn't special case requires clauses on the last template parameter list ...

Indeed, good catch.

@jakobandersen
Copy link
Contributor

I didn't see anything in the Itanium ABI regarding requires clauses --- not sure if you just invented a convention for them or if you used a convention defined elsewhere. With this change, I just used a convention that seemed to make sense.

Right, to my understanding it hasn't been updated officially yet, so this is just something I cooked up, and your changes are fine.

jakobandersen added a commit that referenced this pull request Jul 24, 2022
C++, improve requires clause support (#10286 update)
@jakobandersen
Copy link
Contributor

I have rebased and updated this PR and merged it as #10703. Thanks again for your nice work.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
domains:cpp type:enhancement enhance or introduce a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants