-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Create compilation target versions of ::alloc::Layout
#64299
Conversation
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
6520170
to
ed66767
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This doesn't seem like something I should be reviewing. r? @oli-obk I think you have better context for this? |
I think I found what I changed to cause those issues regarding |
ed66767
to
6392f3a
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #64456) made this pull request unmergeable. Please resolve the merge conflicts. |
So I did a first shallow review, the general idea looks good to me. Wrt the test failures, I think the best way to figure out what causes them is to bisect your own PR. So take the middle commit, see if that passes tests, if it does, take the commit between the currently working commit and the last commit. Repeat until failure. Once you know which commit causes the failure, it will be much simpler to figure out the problem. |
Commenting this for the record: git bisect shows the first bad commit is
I, still, have no idea what's causing this, the failure in CI, or why they are different. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spotted two cases of different behaviour, which may explain the problem.
So those changes weren't the cause of the local failures, but I did a rebase and finished up changing |
6392f3a
to
b33f045
Compare
Huh... you can test master without failures though, right? |
On master the only failure is "---- [codegen] codegen/issue-44056-macos-tls-align.rs stdout ----" |
☔ The latest upstream changes (presumably #64513) made this pull request unmergeable. Please resolve the merge conflicts. |
8270ea4
to
322df44
Compare
So, I haven't found out what's causing the local failures, but I did do a full re-clone of this PR branch into a new directory, and in the new clone the only failures are "[debuginfo-gdb+lldb] debuginfo/*" tests. Therefore the "ui/*" local failures (which still occur in the original clone) are all due to peculiarities in my clone. |
☔ The latest upstream changes (presumably #64864) made this pull request unmergeable. Please resolve the merge conflicts. |
322df44
to
4779637
Compare
☔ The latest upstream changes (presumably #64419) made this pull request unmergeable. Please resolve the merge conflicts. |
4779637
to
4b61893
Compare
☔ The latest upstream changes (presumably #64673) made this pull request unmergeable. Please resolve the merge conflicts. |
b9e889b
to
10946b7
Compare
Hi @Dante-Broggi I am unsure how to proceed here. Even if we do this refactoring we are no step closer to the swift repr or to separating stride from size, because it would break all unsafe code relying on the current scheme. Without first resolving the language/stability I do not believe we can merge this PR or any other moving in this direction |
☔ The latest upstream changes (presumably #60026) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping from triage: |
Marking this as blocked. |
Given the fact this is blocked from long time and based on the comment #64299, it will take a while and the conflicts have accumulated enough to make this a mess when ready. Hence closing this and we can restart it later when ready if possible as it would be easier than rebasing this. Thanks for contributing :) |
This PR adds 2 types
LayoutPositionPref
andMemoryPosition
which are like::alloc::Layout
but uselayout::Size
andlayout::{AbiAndPrefAlign, Align}
, and replaces most pairs ofSize
and*Align
with one of them.Most of the commits are semi-minimal changes after changing a single (group of) APIs, which is why there are so many of them. If desired they could be squashed.
Edit: also, each commit (in order) satisfies at least “./x.py check -i”.
Overall, other than API breakage, this is almost a pure refactoring, the only difference should be that I changed many places to be more correct for non-stride (
.size != .size.aligned_to(.align)
) sizes, which IIRC Rust currently does not allow anyway.