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

Coherence: terminology, rationale, alternatives considered #624

Merged
merged 25 commits into from
Dec 9, 2021
Merged

Coherence: terminology, rationale, alternatives considered #624

merged 25 commits into from
Dec 9, 2021

Conversation

josh11b
Copy link
Contributor

@josh11b josh11b commented Jul 2, 2021

Add an appendix with the rationale for and alternatives to coherence, along with an entry in the terminology and updates to the coherence discussion in the goals.

@josh11b josh11b requested review from a team as code owners July 2, 2021 23:46
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Jul 2, 2021
@josh11b josh11b changed the title Coherence: terminology, rational, and alternatives considered Coherence: terminology, rationale, alternatives considered Jul 2, 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.

Really nice!

I've left a specific comment around the structure below, but the content makes a lot of sense to me and I think does a great job of capturing the tradeoffs and challenges here.

docs/design/generics/appendix-coherence.md Outdated Show resolved Hide resolved
docs/design/generics/appendix-coherence.md Outdated Show resolved Hide resolved
Furthermore, this means that the behavior of a file can depend on an import even
if nothing from that package is referenced explicitly.

## Rejected alternative: dynamic implementation binding
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me from the title of the section what this is an alternative to...

It feels like we're rejecting all forms of incoherence, including an incoherent implementation that uses this alternative's strategy for resolving the challenges. Is my understanding correct?

If so, maybe sink this down a level of the TOC? And clarify above that all of the incoherent alternatives are rejected in some way?

Or maybe I've misunderstood the point a bit? Maybe there is another way to structure/name the sections?

(To be clear, this isn't really an issue with the content, just the structure / headings)

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 redone the section headings. Is this what you were looking for?

body of the `SomethingWeirdHappens` function.

This idea is discussed briefly in section 5.4 on separate compilation of
[this proposal for implementing "Indiana" C++0x concepts proposal](https://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.86.9526&rep=rep1&type=pdf).
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a WG21 link for this paper as well: https://wg21.link/n1848

Unsure which link is best really...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to include both.

@github-actions
Copy link

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please comment or remove the inactive label.
This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

@github-actions github-actions bot added the inactive Issues and PRs which have been inactive for at least 90 days. label Nov 11, 2021
@github-actions
Copy link

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active or becomes active again, please reopen it.
This PR was closed and archived because there has been no new activity in the 14 days since the inactive label was added.

@github-actions github-actions bot closed this Nov 25, 2021
@josh11b josh11b reopened this Nov 29, 2021
@josh11b
Copy link
Contributor Author

josh11b commented Nov 29, 2021

I am still interested in feedback here -- I believe I wrote extra because I wasn't sure what the real alternatives were. It would be great to cut it down to just what is needed.

@josh11b josh11b removed the inactive Issues and PRs which have been inactive for at least 90 days. label Nov 29, 2021
docs/design/generics/goals.md Outdated Show resolved Hide resolved
docs/design/generics/appendix-coherence.md Outdated Show resolved Hide resolved
docs/design/generics/appendix-coherence.md Outdated Show resolved Hide resolved
docs/design/generics/appendix-coherence.md Outdated Show resolved Hide resolved
- It requires more data space at runtime because we need to store a pointer to
the witness table representing the implementation with the object, since it
varies instead of being known statically.
- It is slower to execute from dynamic dispatch and the inability to inline.
Copy link
Contributor

Choose a reason for hiding this comment

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

... and dynamic dispatch may not even be feasible in all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What cases are you thinking of?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this things like we need to know the size of the type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, things like that to do with how we would generate code. I had in mind cases where, for example, a method in the interface returns an associated type, and we don't know the calling convention of the function without knowing some details about the type.

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 added that example.

docs/design/generics/goals.md Outdated Show resolved Hide resolved
@josh11b josh11b merged commit 526172f into carbon-language:trunk Dec 9, 2021
@josh11b josh11b deleted the coherence branch December 9, 2021 21:27
chandlerc added a commit that referenced this pull request Jun 28, 2022
Add an appendix with the rationale for and alternatives to coherence, along with an entry in the terminology and updates to the coherence discussion in the goals.

Co-authored-by: Chandler Carruth <[email protected]>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants