Fix some clippy lints for library/std/src/sys/windows #118154

ChrisDenton · 2023-11-22T00:36:01Z

These issues were shown by running x clippy on library/std and filtering for windows/ paths. I think running clippy on the full std would be great but I wanted to start smaller and with something that's hopefully easier to review. It'd be good to eventually run clippy in CI but that's a bigger conversation.

I've created separate commits for each clippy lint fixed (with the commit title set to the lint name) and reviewed the changes myself. Most of the fixes here are trivial.

r? libs

We intentional use the Windows API style here.

ChrisDenton · 2023-11-22T00:45:13Z

I was so busy playing with clippy that I forgot fmt.

matthiaskrgr · 2023-11-22T01:26:54Z

should definitely get a perf run when you think its ready.

ChrisDenton · 2023-11-22T01:43:39Z

A perf run is very unlikely to show any changes seeing as this is Windows only. Also I'm not really convinced any of these lints would affect perf. I think the only ones that might potentially (and I'm sceptical) are unnecessary_lazy_evaluations, manual_slice_size_calculation, duration_subsec and manual_range_contains. Of those, only manual_range_contains is in perf sensitive code (utf-8 <--> utf-16 conversion).

unneeded `return` statement

This is where our Windows API bindings previously (and incorrectly) used `*mut` instead of `*const` pointers. Now that the bindings have been corrected, the mutable references (which auto-convert to `*mut`) are unnecessary and we can use shared references.

the borrowed expression implements the required traits

this expression creates a reference which is immediately dereferenced by the compiler

casting to the same type is unnecessary

calling `subsec_micros()` is more concise than this calculation

unnecessary closure used with `bool::then`

manual implementation of `Option::map`

taken reference of right operand

comparing with null is better expressed by the `.is_null()` method

ChrisDenton · 2023-11-22T14:08:24Z

library/std/src/sys/windows/stdio.rs

-        if first_code_unit_remaining >= 0xDCEE && first_code_unit_remaining <= 0xDFFF {
+        if matches!(first_code_unit_remaining, 0xDCEE..=0xDFFF) {


I confirmed that using matches! is exactly equivalent to using a manual if (in terms of codegen) and I do think it's clearer.

Yup, so long as there's only a single range in the matches! then it's just as good.

(If there's multiple then things get complicated, see #103024 (comment) if you're curious.)

scottmcm · 2023-11-22T22:30:52Z

library/std/src/sys/windows/fs.rs

@@ -156,7 +156,7 @@ impl DirEntry {
    }

    pub fn path(&self) -> PathBuf {
-        self.root.join(&self.file_name())
+        self.root.join(self.file_name())


Can we uplift this clippy lint? 🙂

(Not in this PR, obviously)

That's needless_borrows_for_generic_args. There's also the related needless_borrow which found a lot too!

scottmcm · 2023-11-22T22:34:23Z

library/std/src/sys/windows/io.rs

@@ -36,7 +36,7 @@ impl<'a> IoSlice<'a> {

    #[inline]
    pub fn as_slice(&self) -> &[u8] {
-        unsafe { slice::from_raw_parts(self.vec.buf as *mut u8, self.vec.len as usize) }


Hmm, this is PSTR, which is defined as *mut u8, so the change is fine, but I wonder if it's "really" *mut u8 vs *mut c_char, since the latter might want to be signed char, and where this cast would actually do something.

Since it's not using c_char today, though, it's probably fine, since casts can be added back if it's ever relevant.

That's true. Though we're a bit at the mercy of the win32metadata project here (which is what our bindings are ultimately generated from) which might not necessarily follow what we think a c_char is (as it has no particular need to follow C).

scottmcm · 2023-11-22T22:39:19Z

This all looks good to me! Nice work.

r? @scottmcm
@bors r+ rollup=iffy (it's all windows, so the PR build doesn't check much)

bors · 2023-11-22T22:39:22Z

📌 Commit b9fe367 has been approved by scottmcm

It is now in the queue for this repository.

bors · 2023-11-22T23:24:06Z

⌛ Testing commit b9fe367 with merge f3c5bff...

Fix some clippy lints for library/std/src/sys/windows These issues were shown by running `x clippy` on `library/std` and filtering for `windows/` paths. I think running clippy on the full std would be great but I wanted to start smaller and with something that's hopefully easier to review. It'd be good to eventually run clippy in CI but that's a bigger conversation. I've created separate commits for each clippy lint fixed (with the commit title set to the lint name) and reviewed the changes myself. Most of the fixes here are trivial. r? libs

rust-log-analyzer · 2023-11-23T00:02:19Z

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

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

[RUSTC-TIMING] rustc_codegen_ssa test:false 102.204
   Compiling rustc_hir_typeck v0.0.0 (/checkout/compiler/rustc_hir_typeck)
[RUSTC-TIMING] rustc_codegen_llvm test:false 87.695
   Compiling rustc_mir_transform v0.0.0 (/checkout/compiler/rustc_mir_transform)
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
##[group]Clock drift check
##[group]Clock drift check
Session terminated, killing shell... ...killed.
  network time: 
##[error]The operation was canceled.
Cleaning up orphan processes

bors · 2023-11-23T00:02:50Z

💔 Test failed - checks-actions

ChrisDenton · 2023-11-23T00:06:13Z

@bors retry that looks spurious "[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled."

bors · 2023-11-23T00:30:18Z

⌛ Testing commit b9fe367 with merge c387f01...

bors · 2023-11-23T02:25:45Z

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing c387f01 to master...

rust-timer · 2023-11-23T06:25:14Z

Finished benchmarking commit (c387f01): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.896s -> 674.393s (-0.22%)
Artifact size: 313.66 MiB -> 313.69 MiB (0.01%)

allow clippy style in windows/c.rs

ad12be3

We intentional use the Windows API style here.

rustbot assigned joshtriplett Nov 22, 2023

rustbot added O-windows Operating system: Windows 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 Nov 22, 2023

This comment has been minimized.

Sign in to view

ChrisDenton force-pushed the win-clippy branch from 0c38f22 to 1cd6dcd Compare November 22, 2023 01:21

ChrisDenton added 12 commits November 22, 2023 13:00

needless_return

533de2b

unneeded `return` statement

useless_conversion

6c22e57

unnecessary_mut_passed

bfbeb3e

This is where our Windows API bindings previously (and incorrectly) used `*mut` instead of `*const` pointers. Now that the bindings have been corrected, the mutable references (which auto-convert to `*mut`) are unnecessary and we can use shared references.

manual_slice_size_calculation

fe25569

needless_borrows_for_generic_args

4273459

the borrowed expression implements the required traits

needless_borrow

2454263

this expression creates a reference which is immediately dereferenced by the compiler

unnecessary_cast

9e42456

casting to the same type is unnecessary

duration_subsec

b962ae1

calling `subsec_micros()` is more concise than this calculation

redundant_closure

220217a

unnecessary_lazy_evaluations

8c85c5b

unnecessary closure used with `bool::then`

manual_map

4c084c5

manual implementation of `Option::map`

op_ref

d7e1f1c

taken reference of right operand

ChrisDenton force-pushed the win-clippy branch from 1cd6dcd to 0e4f04c Compare November 22, 2023 13:01

ChrisDenton added 4 commits November 22, 2023 13:17

manual_range_contains

c15adf6

cmp_null

852c038

comparing with null is better expressed by the `.is_null()` method

redundant_slicing

6c8ebf1

x fmt library/std

b9fe367

ChrisDenton force-pushed the win-clippy branch from 0e4f04c to b9fe367 Compare November 22, 2023 14:03

ChrisDenton commented Nov 22, 2023

View reviewed changes

scottmcm reviewed Nov 22, 2023

View reviewed changes

rustbot assigned scottmcm and unassigned joshtriplett Nov 22, 2023

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 Nov 22, 2023

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

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 Nov 23, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 23, 2023

bors merged commit c387f01 into rust-lang:master Nov 23, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Nov 23, 2023

ChrisDenton deleted the win-clippy branch November 23, 2023 02:32

This was referenced Nov 23, 2023

Add IoSlice::as_bytes #111277

Closed

Move platform modules into sys::pal #117285

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some clippy lints for library/std/src/sys/windows #118154

Fix some clippy lints for library/std/src/sys/windows #118154

ChrisDenton commented Nov 22, 2023

This comment has been minimized.

ChrisDenton commented Nov 22, 2023

matthiaskrgr commented Nov 22, 2023

ChrisDenton commented Nov 22, 2023

ChrisDenton Nov 22, 2023

scottmcm Nov 22, 2023

scottmcm Nov 22, 2023

ChrisDenton Nov 22, 2023

scottmcm Nov 22, 2023

ChrisDenton Nov 22, 2023

scottmcm commented Nov 22, 2023

bors commented Nov 22, 2023

bors commented Nov 22, 2023

rust-log-analyzer commented Nov 23, 2023

bors commented Nov 23, 2023

ChrisDenton commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

rust-timer commented Nov 23, 2023

		if first_code_unit_remaining >= 0xDCEE && first_code_unit_remaining <= 0xDFFF {
		if matches!(first_code_unit_remaining, 0xDCEE..=0xDFFF) {

Fix some clippy lints for library/std/src/sys/windows #118154

Fix some clippy lints for library/std/src/sys/windows #118154

Conversation

ChrisDenton commented Nov 22, 2023

This comment has been minimized.

ChrisDenton commented Nov 22, 2023

matthiaskrgr commented Nov 22, 2023

ChrisDenton commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm commented Nov 22, 2023

bors commented Nov 22, 2023

bors commented Nov 22, 2023

rust-log-analyzer commented Nov 23, 2023

bors commented Nov 23, 2023

ChrisDenton commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

rust-timer commented Nov 23, 2023

Overall result: no relevant changes - no action needed