Rename align to default_align on Scalar and Primitive#158542
Conversation
|
Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE machinery
cc @bjorn3 |
rustbot
added
A-LLVM
|
|
This comment has been minimized.
This comment has been minimized.
scottmcm
force-pushed
the
scalar-but-packed
branch
from
4edeed7 to
2aaf9e0
Compare
|
cc @rust-lang/miri |
| /// | ||
| /// This is the type alignment for the corresponding built-in. | ||
| /// In other contexts it might have different alignment. | ||
| pub fn platform_align<C: HasDataLayout>(self, cx: &C) -> AbiAlign { |
There was a problem hiding this comment.
Naming bikeshed: what about default_align?
There was a problem hiding this comment.
Sounds good -- better conveys "might not the the default" elsewhere than "platform" does since of course we're always targeting just one platform.
To emphasize that just because you see a `Scalar(I32)` that doesn't really tell you anything about the alignment it has -- one should be looking at the type (well, the place) for that. No actual layout or behaviour changes in *this* PR.
scottmcm
force-pushed
the
scalar-but-packed
branch
from
2aaf9e0 to
f98365f
Compare
scottmcm
changed the title
align to platform_align on Scalar and Primitivealign to default_align on Scalar and Primitive
Co-authored-by: Ralf Jung <post@ralfj.de>
| /// | ||
| /// This is the type alignment for the corresponding built-in. | ||
| /// In other contexts it might have different alignment. | ||
| pub fn default_align<C: HasDataLayout>(self, cx: &C) -> AbiAlign { |
There was a problem hiding this comment.
should we be changing this into Align now that we're diffing every line where this is called anyways?
(feel free to answer "no", I haven't checked whether that's a reasonable, nevermind good idea. I only raise it because it should be functionally no-op)
There was a problem hiding this comment.
I think I'm inclined to leave it matching https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.LayoutData.html#structfield.align as AbiAlign for now. Arguably the default alignment is an ABI alignment (as opposed to a preferred one or an arbitrary one) so it's not unreasonable.
And really I'm hoping to actually just delete the vast majority of these anyway by not needing to do that align_to all over the place anyway 🙂
I'll add it to the list of things to think about if I can't just delete it, though.
There was a problem hiding this comment.
I started on the next PR to do this:
- ScalarPair(Scalar, Scalar),
+ ScalarPair {
+ a: Scalar,
+ b: Scalar,
+ b_offset: Size,
+ },and it looks like it removes all the default_align calls from codegen and CTFE -- it's only left in rustc_abi and the checks in compiler/rustc_ty_utils\src\layout\invariant.rs (the latter of which the MCP would probably also remove).
So looking promising.
|
@bors r=workingjubilee rollup |
rust-bors
Bot
added
S-waiting-on-bors
…ingjubilee Rename `align` to `default_align` on `Scalar` and `Primitive` To emphasize that just because you see a `Scalar(I32)` that doesn't really tell you anything about the alignment it has -- one should be looking at the type (well, the place) for that. (The `size` method doesn't really have that problem -- i32 is always 4 bytes, no matter the context -- so is left untouched.) No actual layout or behaviour changes in *this* PR. Just the rename and some comments. r? @workingjubilee
Rollup of 6 pull requests Successful merges: - #157718 (Do not increase depth when evaluating nested goals of `NormalizesTo`) - #158449 (QNX target renaming) - #158483 (signed strict division: just use normal division) - #158516 ( Deduplicate codegen backends in bootstrap config) - #158542 (Rename `align` to `default_align` on `Scalar` and `Primitive`) - #158636 (linkchecker: upgrade to `html5ever v0.39`)
Rollup merge of #158542 - scottmcm:scalar-but-packed, r=workingjubilee Rename `align` to `default_align` on `Scalar` and `Primitive` To emphasize that just because you see a `Scalar(I32)` that doesn't really tell you anything about the alignment it has -- one should be looking at the type (well, the place) for that. (The `size` method doesn't really have that problem -- i32 is always 4 bytes, no matter the context -- so is left untouched.) No actual layout or behaviour changes in *this* PR. Just the rename and some comments. r? @workingjubilee
Rollup of 6 pull requests Successful merges: - rust-lang/rust#157718 (Do not increase depth when evaluating nested goals of `NormalizesTo`) - rust-lang/rust#158449 (QNX target renaming) - rust-lang/rust#158483 (signed strict division: just use normal division) - rust-lang/rust#158516 ( Deduplicate codegen backends in bootstrap config) - rust-lang/rust#158542 (Rename `align` to `default_align` on `Scalar` and `Primitive`) - rust-lang/rust#158636 (linkchecker: upgrade to `html5ever v0.39`)
…ingjubilee Rename `align` to `default_align` on `Scalar` and `Primitive` To emphasize that just because you see a `Scalar(I32)` that doesn't really tell you anything about the alignment it has -- one should be looking at the type (well, the place) for that. (The `size` method doesn't really have that problem -- i32 is always 4 bytes, no matter the context -- so is left untouched.) No actual layout or behaviour changes in *this* PR. Just the rename and some comments. r? @workingjubilee
5 participants
To emphasize that just because you see a
Scalar(I32)that doesn't really tell you anything about the alignment it has -- one should be looking at the type (well, the place) for that.(The
sizemethod doesn't really have that problem -- i32 is always 4 bytes, no matter the context -- so is left untouched.)No actual layout or behaviour changes in this PR. Just the rename and some comments.
r? @workingjubilee