prefer T::IS_ZST over manual check

[Lars-Schumann]

makes the intent clearer and possible small perf improvement

[mejrs]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: b37c1ea (b37c1ead988f55420588e0368e2cd2b36ed1fb08, parent: c7fe5e9d1eb0ef8cc033961956d60f8e2e91e741)

[rust-timer]

Finished benchmarking commit (b37c1ea): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 1.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.9%	[1.9%, 1.9%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.9%	[1.9%, 1.9%]	1

Cycles

Results (primary 3.8%, secondary 2.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.8%	[3.4%, 4.1%]	2
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.8%	[3.4%, 4.1%]	2

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 488.641s -> 495.724s (1.45%)
Artifact size: 393.90 MiB -> 393.88 MiB (-0.01%)

[teor2345]

There are also some instances of this check in the compiler itself, and some ZST checks that are spelt differently.

There's still these instances in the library:

• compiler_builtins: const { assert!(mem::size_of::<T>() != 0) };
• proc_macro: assert!(size_of::<F>() == 0, ...

And the compiler:

• rustc_middle: assert!(size_of::<T>() != 0);
• rustc_arena:
  • if size_of::<T>() == 0
  • assert!(sizeof::<T>() != 0)
  • assert_ne!(sizeof::<T>(), 0) after Disallow ZST allocations with TypedArena. #155326

There's also a bunch in src/gcc/libgrust/rustc-lib, but I don't know if it defines IS_ZST.

[ChrisDenton]

Rather than enabling this whole feature, would it be better for the compiler to have it's own IsZst trait specifically for IS_ZST?

View changes since the review

[Lars-Schumann]

sized_type_properties is a pretty small feature, it only gates the SizedTypeProperties trait which does nothing but define a few constants. I don't know what rustc's policy on avoiding unstable features is, if it is an issue I'm happy to make such a trait.

[rustbot]

Portable SIMD is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead.

cc @calebzulawski, @programmerjake

compiler-builtins is developed in its own repository. If possible, consider making this change to rust-lang/compiler-builtins instead.

cc @tgross35

[rustbot]

r? @ShoyuVanilla

rustbot has assigned @ShoyuVanilla.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: arena, compiler
• arena, compiler expanded to 73 candidates
• Random selection from 20 candidates

[Lars-Schumann]

Portable SIMD is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead.

compiler-builtins is developed in its own repository. If possible, consider making this change to rust-lang/compiler-builtins instead.

Oh, great that this pings the world :], I assume I just remove the relevant changes here and make separate PRs to those repos?

[tgross35]

What is the reason for this change? The top post says clearer intent and possible perf improvement, but it has a comment and it's in const.

[Lars-Schumann]

We want to assure T is not a ZST, !T::IS_ZST is easier to read than mem::size_of::<T>() != 0) and the canonical way to make this check.
Regarding the comment, it seems to be redundant with this change, we could remove it.

[tgross35]

This changes seems fine for anything already using the feature (core/alloc/std), but using a very-internal standard library API anywhere else doesn't seem worth it. It's not that much more readable and it certainly isn't canonical in the ecosystem. I'd like to second @ChrisDenton's suggestion of a decoupled trait for the compiler if the change is really worth it.

At a minimum please drop the subtree changes, it's better to minimize unstable features where possible so syncs are less painful if things change.