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

Updating function and variable docs #1017

Merged
merged 18 commits into from
Jan 28, 2022
Merged

Updating function and variable docs #1017

merged 18 commits into from
Jan 28, 2022

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Jan 12, 2022

I was doing this for #851 initially, but I think #438 and #826 hadn't made it in (possibly intentionally due to #851? I don't recall). Feel free to comment on structure, I was trying to decide how best to approach this.

@jonmeow jonmeow requested a review from zygoloid January 12, 2022 01:39
@jonmeow jonmeow requested review from a team as code owners January 12, 2022 01:39
docs/design/functions.md Show resolved Hide resolved
docs/design/type_inference.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@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.

Addressing comments

docs/design/type_inference.md Outdated Show resolved Hide resolved
docs/design/functions.md Show resolved Hide resolved
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.

Definitely good to freshen all of this. Pulling in @wolffg explicitly here as he's been thinking about these high-level design concerns and may have wording or structure suggestions. It looks OK to me, but I also want to make sure this is working well for other readers as well. I have some comments inline too.

docs/design/functions.md Outdated Show resolved Hide resolved
docs/design/functions.md Outdated Show resolved Hide resolved
docs/design/functions.md Show resolved Hide resolved

<!-- tocstop -->

## Overview
Copy link
Contributor

Choose a reason for hiding this comment

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

@zygoloid to ensure we're doing a decent job of introducing type inference and not missing any implications or connections to other parts of the language that we should establish...

Copy link
Contributor Author

@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.

Comment pass

docs/design/functions.md Show resolved Hide resolved
docs/design/functions.md Outdated Show resolved Hide resolved
docs/design/functions.md Outdated Show resolved Hide resolved
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.

This basically looks good to me. Some minor suggestions inline. I've also pinged on one comment thread that I'd at least like to get an ACK on before this lands.

docs/design/functions.md Outdated Show resolved Hide resolved
docs/design/variables.md Outdated Show resolved Hide resolved
jonmeow and others added 2 commits January 27, 2022 12:57
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Copy link
Contributor Author

@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.

Rephrasing return statements.

docs/design/functions.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 great to me. Some minor wording suggestions.

docs/design/functions.md Outdated Show resolved Hide resolved
Comment on lines 131 to 133
A function declaration can be called, and the function definition may be
provided later in code; for example, the `api` file of a library will often
contain a function's declaration while the `impl` file contains the definition.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a bit repetitive with things in the previous paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

docs/design/functions.md Outdated Show resolved Hide resolved
docs/design/variables.md Outdated Show resolved Hide resolved
jonmeow and others added 6 commits January 28, 2022 10:10
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
- For example, `fn Sleep(seconds: i64);` is similar to
`fn Sleep(seconds: i64) -> ();`.
- `()` is similar to a `void` return type in C++.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps state that switching between fn F(...) and fn F(...) -> () is a compatible change for all callers? (though the return statements in the function's definition would have to be updated)

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'd suggest proposing this separately. There are two reasons I could see this not being true:

  1. It could be decided that var x: auto = F() is either required or disallowed in some cases.
  2. Constraints could be allowed to detect the difference.

In any case, I think this is a non-trivial decision and may be more appropriate for a proposal or its own change (this doc edit has already taken longer than I expected, I'd rather not push it out another couple weeks).

@jonmeow jonmeow merged commit 33ede71 into carbon-language:trunk Jan 28, 2022
@jonmeow jonmeow deleted the var-docs branch January 28, 2022 19:36
chandlerc added a commit that referenced this pull request Jun 28, 2022
I was doing this for #851 initially, but I think #438 and #826 hadn't made it in (possibly intentionally due to #851? I don't recall). 

Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
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.

4 participants