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

Generics overview #524

Merged
merged 26 commits into from
Jun 29, 2021
Merged

Generics overview #524

merged 26 commits into from
Jun 29, 2021

Conversation

josh11b
Copy link
Contributor

@josh11b josh11b commented May 6, 2021

This adds an overview of a generics feature that attempts to achieve the goals from #24 . It has been summarized in these presentations:

@josh11b josh11b added the proposal A proposal label May 6, 2021
@josh11b josh11b requested a review from a team May 6, 2021 20:43
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label May 6, 2021
@github-actions github-actions bot added the proposal rfc Proposal with request-for-comment sent out label May 6, 2021
@josh11b josh11b marked this pull request as ready for review May 6, 2021 21:08
@josh11b josh11b requested a review from a team as a code owner May 6, 2021 21:08
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Through the doc, there's some syntax that doesn't reflect current Carbon syntax. Things like Ptr slow me down as I try to read this doc, as I need to mentally translate syntax.

I see a lot of discussion of Carbon's template behavior and syntax. On #24 I felt there was pushback on handling template design, however, this doc seems to be going in the other direction. This creates a tension for me of not being sure what you're trying to design here.

Having reached "implicit parameters" and [Int n] syntax (which appears to be neither generic nor template syntax), I now feel out of depth on this design. I don't feel comfortable reviewing syntax for things that aren't being discussed.

Noting that approval of the doc implies approval of the contained design decisions, I would personally prefer for this to be tightened up more to talk more specifically about generics, and avoid discussion of other potential features, unless all features contained are going to be justified. As it is, I don't feel comfortable reviewing further because I lack sufficient background to review.

docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
@josh11b
Copy link
Contributor Author

josh11b commented May 26, 2021

Through the doc, there's some syntax that doesn't reflect current Carbon syntax. Things like Ptr slow me down as I try to read this doc, as I need to mentally translate syntax.

I will update the syntax soon, seems like there are a number of relevant issues that are getting close to decisions.

I see a lot of discussion of Carbon's template behavior and syntax. On #24 I felt there was pushback on handling template design, however, this doc seems to be going in the other direction. This creates a tension for me of not being sure what you're trying to design here.

I removed most of the template content, except a few things contrasting generics with templates.

Having reached "implicit parameters" and [Int n] syntax (which appears to be neither generic nor template syntax), I now feel out of depth on this design. I don't feel comfortable reviewing syntax for things that aren't being discussed.

This syntax is more general than generics, and is introduced as a more general feature, but we wouldn't have it except for the use cases for generics.

Noting that approval of the doc implies approval of the contained design decisions, I would personally prefer for this to be tightened up more to talk more specifically about generics, and avoid discussion of other potential features, unless all features contained are going to be justified. As it is, I don't feel comfortable reviewing further because I lack sufficient background to review.

Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Tried to make a pass over all of this. I feel like I may have missed some stuff, but can catch that after a revision.

docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
@josh11b
Copy link
Contributor Author

josh11b commented Jun 9, 2021

This PR is on hold while @wolffg and I work on a significant restructuring.

Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

I said it while I was going through things, but just wanted to lead with this is well: I think this is a really fantastic overview document. Super impressed by how approachable it is and yet how much it manages to cover. Really nicely done.

Most of my comments below are pretty minor attempts to tweak and improve wording. I don't actually have any substantive concerns here. Leaving open mostly to let some others take a look and get an official stamp from leads on this since I think this really gets the core of everything about generics outlined. I'm roughly 99% sure there is consensus here already, but seems good to confirm.

docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Show resolved Hide resolved
@chandlerc chandlerc self-assigned this Jun 25, 2021
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

LGTM, this seems good to land!

docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
docs/design/generics/overview.md Outdated Show resolved Hide resolved
@josh11b josh11b merged commit 57a376f into carbon-language:trunk Jun 29, 2021
@josh11b josh11b deleted the generics-overview branch June 29, 2021 17:52
@github-actions github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Jun 29, 2021
chandlerc added a commit that referenced this pull request Jun 28, 2022
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.

5 participants