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

Generic impls access (details 4) #931

Merged
merged 11 commits into from
Dec 7, 2021
Merged

Generic impls access (details 4) #931

merged 11 commits into from
Dec 7, 2021

Conversation

josh11b
Copy link
Contributor

@josh11b josh11b commented Oct 29, 2021

Implementations of interfaces are as public as the names used in their signature. No access control modifiers are allowed on impl declarations.

@josh11b josh11b added the proposal A proposal label Oct 29, 2021
@josh11b josh11b requested a review from a team October 29, 2021 23:16
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Oct 29, 2021
@josh11b josh11b marked this pull request as ready for review October 29, 2021 23:55
@josh11b josh11b requested a review from a team as a code owner October 29, 2021 23:55
@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Oct 30, 2021
@josh11b josh11b assigned zygoloid and unassigned zygoloid Nov 16, 2021
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

Generally the direction of using adapters instead of having some notion of private impl seems good to me.

docs/design/generics/details.md Outdated Show resolved Hide resolved
docs/design/generics/details.md Outdated Show resolved Hide resolved
proposals/p0931.md Outdated Show resolved Hide resolved
docs/design/generics/details.md Outdated Show resolved Hide resolved
proposals/p0931.md Outdated Show resolved Hide resolved
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

Looks good to me, checking with other leads.

Comment on lines +85 to +87
library full control. We considered and rejected the idea that developers could
put that interface declaration in an API file to allow it to be referenced in
named constraints available to users. All impls for that interface would also
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the rejection of this reflected in existing design documents somewhere? This seems like something that would be allowed by default unless we expressly prohibit it, and at least this proposal's design updates don't seem to do so. I'm OK with this rejected alternative being the only record of this for now, especially given that this is listed as something we might want to revisit, but please file an issue to track this so we don't forget entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created #971 with this question.

Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

Please proceed :)

@josh11b josh11b merged commit a34f2d7 into carbon-language:trunk Dec 7, 2021
@josh11b josh11b deleted the access branch December 7, 2021 17:28
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Dec 7, 2021
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
Implementations of interfaces are as public as the names used in their signature. No access control modifiers are allowed on `impl` declarations.

Co-authored-by: Richard Smith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot. proposal accepted Decision made, proposal accepted proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants