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.

Sign up for GitHub

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

Jump to bottom

Exporting imported names #3938

Merged
merged 9 commits into from
May 16, 2024
Merged

Exporting imported names #3938

merged 9 commits into from
May 16, 2024

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented May 6, 2024 •
edited

In order to support exporting imported names, add export import library <library> and export <name reference> syntax.

@jonmeow jonmeow added proposal A proposal proposal draft Proposal in draft, not ready for review labels May 6, 2024
@jonmeow jonmeow force-pushed the proposal-export branch 2 times, most recently from 63b76a6 to 63f2a56 Compare May 6, 2024 23:29
@jonmeow jonmeow changed the title export Exporting imported names May 6, 2024
@jonmeow jonmeow force-pushed the proposal-export branch 2 times, most recently from a0d816c to faaf653 Compare May 6, 2024 23:32
@jonmeow jonmeow marked this pull request as ready for review May 6, 2024 23:33
@github-actions github-actions bot requested a review from KateGregory May 6, 2024 23:34
@github-actions github-actions bot added proposal rfc Proposal with request-for-comment sent out and removed proposal draft Proposal in draft, not ready for review labels May 6, 2024
@jonmeow
Copy link
Contributor Author

jonmeow commented May 6, 2024

Note, not sure what the best way is to see if people agree on the syntax choice -- I was thinking if there's a preference for an alternative, I can adjust the proposal. But export import at least sounded like people though it'd be okay, in the discussion.

zygoloid
zygoloid reviewed May 7, 2024
View reviewed changes
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.

LGTM

@jonmeow
Copy link
Contributor Author

jonmeow commented May 7, 2024 •
edited

@zygoloid FYI I want to be sure you notice the namespace commit. I don't think it really changes the proposal, but it I was trying to close some design gaps I realized existed (naively, export NS; would only export the namespace itself, not contents). [also, export OtherPackage.Name; wasn't explicitly disallowed, and is now]

@zygoloid
Copy link
Contributor

zygoloid commented May 7, 2024

Still LG with namespace changes. I agree that disallowing namespace exports for now is the right choice, consistent with the existing rule that namespaces are exported iff they contain anything exported.

zygoloid
zygoloid reviewed May 9, 2024
View reviewed changes
proposals/p3938.md Outdated Show resolved Hide resolved
@jonmeow jonmeow mentioned this pull request May 10, 2024
Merged
@jonmeow jonmeow mentioned this pull request May 15, 2024
Merged
chandlerc
chandlerc reviewed May 15, 2024
View reviewed changes
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, minor wording suggestions below, nothing substantive.

I agree with the option selected for where you can write export on a name. Checking with other leads to see if I can approve...

proposals/p3938.md Outdated Show resolved Hide resolved
proposals/p3938.md Outdated Show resolved Hide resolved
chandlerc
chandlerc approved these changes May 15, 2024
View reviewed changes
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.

And approving with wording fixes.

chandlerc
chandlerc approved these changes May 15, 2024
View reviewed changes
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.

And approving with wording fixes.

jonmeow and others added 2 commits May 16, 2024 08:02
@jonmeow @chandlerc
Update proposals/p3938.md
1fc5ef7 
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
@jonmeow
Addressing comments
99f304f
@chandlerc
Copy link
Contributor

And merging (confirmed with the author).

@chandlerc chandlerc added this pull request to the merge queue May 16, 2024
Merged via the queue into carbon-language:trunk with commit 122a361 May 16, 2024
7 checks passed
@jonmeow jonmeow deleted the proposal-export branch May 17, 2024 14:50
github-merge-queue bot pushed a commit that referenced this pull request May 17, 2024
@jonmeow
Move 'export name' out of the import section. (#3950)
ecb300e 
This implements recent updates to #3938
CJ-Johnson pushed a commit to CJ-Johnson/carbon-lang that referenced this pull request May 23, 2024
@jonmeow @chandlerc @CJ-Johnson
Exporting imported names (carbon-language#3938)
b51def2 
In order to support exporting imported names, add `export import library
<library>` and `export <name reference>` syntax.

---------

Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
CJ-Johnson pushed a commit to CJ-Johnson/carbon-lang that referenced this pull request May 23, 2024
@jonmeow @CJ-Johnson
Move 'export name' out of the import section. (carbon-language#3950)
0d879bb 
This implements recent updates to carbon-language#3938
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zygoloid zygoloid zygoloid left review comments

@chandlerc chandlerc chandlerc approved these changes

@KateGregory KateGregory Awaiting requested review from KateGregory

Assignees
No one assigned
Labels
proposal rfc Proposal with request-for-comment sent out proposal A proposal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jonmeow @zygoloid @chandlerc