-
Notifications
You must be signed in to change notification settings - Fork 13.5k
tests/ui
: A New Order [21/N]
#143296
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
tests/ui
: A New Order [21/N]
#143296
Conversation
//! Verifies that Option<T> has the same size as T for nullable pointer types, | ||
//! and that custom enums get the same optimization. |
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.
Slight correction
//! Verifies that Option<T> has the same size as T for nullable pointer types, | |
//! and that custom enums get the same optimization. | |
//! Verifies that Option<T> has the same size as T for non-nullable pointer types, | |
//! and for custom enums that have a niche. |
@@ -1,27 +1,34 @@ | |||
//@ run-pass | |||
//! Nullable pointer optimization with iota-reduction for enums. |
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.
The term should be "null pointer optimization", not "nullable"; because types like &T
and Box<T>
are not nullable.
Also for the file name
@@ -1,31 +1,41 @@ | |||
//! Nullable pointer optimization preserves type sizes. |
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.
Nullable -> Null (also the file name)
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.
Few small comments, please also squash!
@rustbot ready |
@@ -1,31 +1,41 @@ | |||
//! null pointer optimization preserves type sizes. | |||
//! | |||
//! Verifies that Option<T> has the same size as T for null pointer types, |
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.
"null" -> "non-nullable" (A strictly-null pointer type would be a ZST :D )
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.
names also should be non-null in both tests?
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.
nvm, just hopes that not, pushed as it replaced null with non-null
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.
So for some context here: NPO (null pointer optimization) is an optimization on non-nullable pointer types (&T
, Box<T>
, etc) using the null representation. (I know, a bit confusing). Basically it knows that &T
has an identical layout to *T
; but &T
can never be 0x0, because that it's a non-nullable type. So that is what's known as a "niche", and it can use that to store something; e.g. for Option<&T>
, 0x0 means None
(since it can't be &T
) and any other value is Some(&T)
.
All that to say that comment should be "...same size as T for non-nullable pointer types", since the type has to be non-nullable for NPO to mean anything (so what you have is 👍). And then the test could be named null-pointer-optimization-sizes.rs
or npo-sizes.rs
(keeping null-pointer-optimization
together rather than putting size
in the middle)
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.
so last changes should be fine, thanks for detailed explanation!
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.
oh, wait need some adjustments
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.
now i believe should be fine
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.
Sorry, I was asking for one last thing 😄
null-pointer-size-optimization.rs
->null-pointer-optimization-sizes.rs
since "null pointer optimization" is a phrase
Looks like it's still at tests/ui/layout/null-pointer-size-optimization.rs
?
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.
oh wait again then! for some reasons this message didnt rendered on my side 0_0
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.
ready, also recheck comments in those tests
50f65e8
to
1778762
Compare
Thank you! |
Rollup of 11 pull requests Successful merges: - #142440 (`tests/ui`: A New Order [14/N]) - #143040 (Add `const Rem`) - #143086 (Update poison.rs to fix the typo (sys->sync)) - #143202 (`tests/ui`: A New Order [18/N]) - #143296 (`tests/ui`: A New Order [21/N]) - #143297 (`tests/ui`: A New Order [22/N]) - #143299 (`tests/ui`: A New Order [24/N]) - #143300 (`tests/ui`: A New Order [25/N]) - #143397 (test passing a `VaList` from rust to C) - #143410 (Block SIMD in transmute_immediate; delete `OperandValueKind`) - #143452 (Fix CLI completion check in `tidy`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #143296 - Kivooeo:tf21, r=tgross35 `tests/ui`: A New Order [21/N] > [!NOTE] > > Intermediate commits are intended to help review, but will be squashed prior to merge. Some `tests/ui/` housekeeping, to trim down number of tests directly under `tests/ui/`. Part of #133895. r? `@tgross35`
Note
Intermediate commits are intended to help review, but will be squashed prior to merge.
Some
tests/ui/
housekeeping, to trim down number of tests directly undertests/ui/
. Part of #133895.r? @tgross35