Migrate lib's `&Option<T>` into `Option<&T>` #130962

nyurik · 2024-09-28T05:49:12Z

Trying out my new lint rust-lang/rust-clippy#13336 - according to the video, this could lead to some performance and memory optimizations.

Basic thoughts expressed in the video that seem to make sense:

&Option<T> in an API breaks encapsulation:
- caller must own T and move it into an Option to call with it
- if returned, the owner must store it as Option internally in order to return it
Performance is subject to compiler optimization, but at the basics, &Option<T> points to memory that has presence flag + value, whereas Option<&T> by specification is always optimized to a single pointer.

rustbot · 2024-09-28T05:49:21Z

r? @cuviper

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

Migrate compiler's `&Option<T>` into `Option<&T>` Similar to rust-lang#130962 but for the compiler. Trying out my new lint rust-lang/rust-clippy#13336 - according to the [video](https://www.youtube.com/watch?v=6c7pZYP_iIE), this could lead to some performance and memory optimizations.

cuviper

I don't think any of these instances will matter for performance, but it seems fine as a bit of polish.

library/std/src/sys/pal/unix/process/process_unix.rs

cuviper · 2024-10-09T00:04:20Z

I think part of my earlier comment slipped notice, regarding get_cwd:

There's another call like this in process_vxworks.rs too.

nyurik · 2024-10-09T00:21:51Z

good catch @cuviper, fixed

library/std/src/sys/pal/unix/process/process_vxworks.rs

Co-authored-by: Josh Stone <cuviper@gmail.com>

nyurik · 2024-10-09T03:28:35Z

no idea what has caused last CI failure, rebasing on main.

rust-log-analyzer · 2024-10-09T04:23:52Z

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

------
 > importing cache manifest from ghcr.io/rust-lang/rust-ci-cache:20950f2ccee3dff53a038adf5c1cf05231c0b30772617126a5f6478a66316a29cba9aead69f7bb0004886d32c1f8e6287542cf6d25130711c82d16a66201d4fe:
------
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
/checkout/src/ci/run.sh: line 58: DISABLE_CI_RUSTC_IF_INCOMPATIBLE: command not found
debug:  configured.
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
  Downloaded boml v0.3.1
   Compiling boml v0.3.1
   Compiling y v0.1.0 (/checkout/compiler/rustc_codegen_gcc/build_system)
    Finished `release` profile [optimized] target(s) in 3.77s
     Running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-codegen/x86_64-unknown-linux-gnu/release/y test --use-system-gcc --use-backend gcc --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc --release --mini-tests --std-tests`
Using system GCC
[BUILD] example
[AOT] mini_core_hello_world
/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc/mini_core_hello_world
abc
---
##[group]Building stage1 library artifacts (x86_64-unknown-linux-gnu)
##[endgroup]
##[group]Building tool rustdoc (stage0 -> stage1, x86_64-unknown-linux-gnu)
##[endgroup]
thread 'core::builder::tests::ci_rustc_if_unchanged_logic' panicked at src/core/builder/tests.rs:260:5:
assertion failed: has_changes == config.download_rustc_commit.is_none()


failures:
    core::builder::tests::ci_rustc_if_unchanged_logic

nyurik · 2024-10-09T04:29:22Z

@cuviper is ci_rustc_if_unchanged_logic somehow related to any of these changes?? This didn't trigger before

cuviper · 2024-10-09T04:55:05Z

It's not you -- see #131434

cuviper · 2024-10-11T22:03:26Z

@bors r+ rollup

bors · 2024-10-11T22:03:28Z

📌 Commit a278f15 has been approved by cuviper

It is now in the queue for this repository.

Rollup of 7 pull requests Successful merges: - rust-lang#124874 (intrinsics fmuladdf{32,64}: expose llvm.fmuladd.* semantics) - rust-lang#130962 (Migrate lib's `&Option<T>` into `Option<&T>`) - rust-lang#131289 (stabilize duration_consts_float) - rust-lang#131310 (Support clobber_abi in MSP430 inline assembly) - rust-lang#131546 (Make unused_parens's suggestion considering expr's attributes.) - rust-lang#131565 (Remove deprecation note in the `non_local_definitions` lint) - rust-lang#131576 (Flatten redundant test module `run_make_support::diff::tests::tests`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#130962 - nyurik:opts-libs, r=cuviper Migrate lib's `&Option<T>` into `Option<&T>` Trying out my new lint rust-lang/rust-clippy#13336 - according to the [video](https://www.youtube.com/watch?v=6c7pZYP_iIE), this could lead to some performance and memory optimizations. Basic thoughts expressed in the video that seem to make sense: * `&Option<T>` in an API breaks encapsulation: * caller must own T and move it into an Option to call with it * if returned, the owner must store it as Option<T> internally in order to return it * Performance is subject to compiler optimization, but at the basics, `&Option<T>` points to memory that has `presence` flag + value, whereas `Option<&T>` by specification is always optimized to a single pointer.

rustbot assigned cuviper Sep 28, 2024

rustbot added O-SGX Target: SGX O-unix Operating system: Unix-like 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 Sep 28, 2024

This comment has been minimized.

Sign in to view

nyurik force-pushed the opts-libs branch from c7a34d8 to 068c6a1 Compare September 28, 2024 06:02

nyurik mentioned this pull request Sep 28, 2024

Migrate compiler's &Option<T> into Option<&T> #130963

Open

nyurik changed the title ~~Change a few &Option<T> into Option<&T>~~ Migrate lib's &Option<T> into Option<&T> Sep 28, 2024

cuviper reviewed Oct 2, 2024

View reviewed changes

library/std/src/sys/pal/unix/process/process_unix.rs Outdated Show resolved Hide resolved

cuviper reviewed Oct 9, 2024

View reviewed changes

library/std/src/sys/pal/unix/process/process_vxworks.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

nyurik and others added 4 commits October 8, 2024 23:26

Change a few &Option<T> into Option<&T>

f2d1edf

Update library/std/src/sys/pal/unix/process/process_unix.rs

d2f93c9

Co-authored-by: Josh Stone <cuviper@gmail.com>

fix ref in process_vxworks.rs

442d766

Update library/std/src/sys/pal/unix/process/process_vxworks.rs

a278f15

Co-authored-by: Josh Stone <cuviper@gmail.com>

nyurik force-pushed the opts-libs branch from 6ab6ee4 to a278f15 Compare October 9, 2024 03:27

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2024

tgross35 mentioned this pull request Oct 12, 2024

Rollup of 7 pull requests #131581

Merged

bors merged commit 02cf62c into rust-lang:master Oct 12, 2024
5 of 6 checks passed

rustbot added this to the 1.83.0 milestone Oct 12, 2024

nyurik deleted the opts-libs branch October 12, 2024 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate lib's `&Option<T>` into `Option<&T>` #130962

Migrate lib's `&Option<T>` into `Option<&T>` #130962

nyurik commented Sep 28, 2024 •

edited

Loading

rustbot commented Sep 28, 2024

This comment has been minimized.

cuviper left a comment

cuviper commented Oct 9, 2024

nyurik commented Oct 9, 2024

This comment has been minimized.

This comment has been minimized.

nyurik commented Oct 9, 2024

rust-log-analyzer commented Oct 9, 2024

nyurik commented Oct 9, 2024

cuviper commented Oct 9, 2024

cuviper commented Oct 11, 2024

bors commented Oct 11, 2024

Migrate lib's &Option<T> into Option<&T> #130962

Migrate lib's &Option<T> into Option<&T> #130962

Conversation

nyurik commented Sep 28, 2024 • edited Loading

rustbot commented Sep 28, 2024

This comment has been minimized.

cuviper left a comment

Choose a reason for hiding this comment

cuviper commented Oct 9, 2024

nyurik commented Oct 9, 2024

This comment has been minimized.

This comment has been minimized.

nyurik commented Oct 9, 2024

rust-log-analyzer commented Oct 9, 2024

nyurik commented Oct 9, 2024

cuviper commented Oct 9, 2024

cuviper commented Oct 11, 2024

bors commented Oct 11, 2024

Migrate lib's `&Option<T>` into `Option<&T>` #130962

Migrate lib's `&Option<T>` into `Option<&T>` #130962

nyurik commented Sep 28, 2024 •

edited

Loading