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

Support vec![const { ... }; n] syntax #133412

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

EFanZh
Copy link
Contributor

@EFanZh EFanZh commented Nov 24, 2024

@rustbot
Copy link
Collaborator

rustbot commented Nov 24, 2024

r? @Noratrieb

rustbot has assigned @Noratrieb.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 24, 2024
@jieyouxu

This comment was marked as off-topic.

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 24, 2024
@rustbot rustbot assigned BurntSushi and unassigned Noratrieb Nov 24, 2024
@Noratrieb

This comment was marked as resolved.

@rustbot rustbot assigned Noratrieb and unassigned BurntSushi Nov 24, 2024
@rust-log-analyzer

This comment has been minimized.

@EFanZh EFanZh force-pushed the vec-of-const-expr-macro branch from 245a28e to f8ec8e2 Compare November 24, 2024 10:15
@Noratrieb
Copy link
Member

Sorry no, this is indeed a libs-api thing, as this is insta-stable and needs an FCP to merge. Assigning to dtolnay who seconded the ACP.
r? dtolnay

@rustbot rustbot assigned dtolnay and unassigned Noratrieb Nov 24, 2024
Comment on lines 3211 to 3212
// SAFETY: If `value` is the result of some const expression, we can make as many
// copies as needed.
Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting argument that I have not seen before. I can't see anything immediately wrong with it, but paging in @rust-lang/opsem for extra scrutiny.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be the case with the current const capabilities, but I'm not 100% sure it holds with every possible extension, namely leaking const allocation to runtime. I think the rules around const object unification would make this a valid execution, but I don't have all the relevant info in cache.

Copy link
Member

Choose a reason for hiding this comment

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

We do intent that consts can still be implemented as many copies of the same thing for codegen. So whatever we do with const allocation should support that.

But it may be good to put a comment in the source here explaining that this is the standard library doing privileged reasoning, and not a stable guarantee.

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 have updated the safety comment. Please let me know if there is anything missing or inaccurate.

}
}

// Avoid calling the destructor of `elem`.
Copy link
Member

Choose a reason for hiding this comment

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

This raises the interesting question whether the destructor should be executed at all for vec![const { ... }; 0]. For 0-length arrays, our current behavior here is rather strange: #79580.

Copy link
Contributor Author

@EFanZh EFanZh Nov 24, 2024

Choose a reason for hiding this comment

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

I tested the array of const expression case, the destructor is not called: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f9ffe1eb972bc415b0e6bf6bf48c18c1. This is consistent with the current implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Would be good to have a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test added.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

@bors
Copy link
Contributor

bors commented Nov 30, 2024

☔ The latest upstream changes (presumably #133533) made this pull request unmergeable. Please resolve the merge conflicts.

@tgross35 tgross35 added S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.