Skip to content

Conversation

eduardosm
Copy link
Contributor

No description provided.

@rustbot
Copy link
Collaborator

rustbot commented Sep 8, 2024

r? @fee1-dead

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

@rustbot rustbot added O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Sep 8, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 8, 2024

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@workingjubilee
Copy link
Member

workingjubilee commented Sep 8, 2024

@eduardosm can you split off the library parts into their own PR?

@eduardosm eduardosm changed the title Remove needless returns detected by clippy Remove needless returns detected by clippy the compiler Sep 8, 2024
@eduardosm
Copy link
Contributor Author

Done, libraries part here

@eduardosm eduardosm changed the title Remove needless returns detected by clippy the compiler Remove needless returns detected by clippy in the compiler Sep 8, 2024
@RalfJung
Copy link
Member

RalfJung commented Sep 8, 2024 via email

@eduardosm
Copy link
Contributor Author

I undid some of the changes, trying to keep only the most obvious ones.

@@ -336,7 +336,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
return true;
}
}
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it is nicer, for symmetry with the return just a few lines above, to use return here.

That's exactly why I don't like just forcing such clippy lints over the entire codebase...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have exactly the opposite opinion. I like the consistency that tools like clippy encourage, and in this case I think the change makes the "early return" and "default/fallthrough" visually distinct, which I find easier to read.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last two returns here are not distinct in a meaningful sense, so it is not a good idea to make them visually distinct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @saethlin. I think it's just plain odd to have return value; at the end of a block when it's not suggesting exceptional control flow.

I think it's actually very odd that miri has allow(clippy::needless_return) on its codebase, but for the rest of the compiler I think this should be fixed.

@compiler-errors

This comment was marked as off-topic.

@compiler-errors
Copy link
Member

compiler-errors commented Sep 11, 2024

Oops, closed the wrong PR !! Meant to close mine which is a duplicate :)

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up implementing the exact same changes, so I basically did a complete review of this by doing the changes myself. This LGTM.

@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Sep 11, 2024

📌 Commit 0b20ffc has been approved by compiler-errors

It is now in the queue for this repository.

@bors 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 Sep 11, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 11, 2024
…iler-errors

Remove needless returns detected by clippy in the compiler
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
…kingjubilee

Rollup of 11 pull requests

Successful merges:

 - rust-lang#119286 (show linker output even if the linker succeeds)
 - rust-lang#129103 (Don't warn empty branches unreachable for now)
 - rust-lang#129696 (update stdarch)
 - rust-lang#129835 (enable const-float-classify test, and test_next_up/down on 32bit x86)
 - rust-lang#129992 (Update compiler-builtins to 0.1.125)
 - rust-lang#130052 (Don't leave debug locations for constants sitting on the builder indefinitely)
 - rust-lang#130077 (Fix linking error when compiling for 32-bit watchOS)
 - rust-lang#130114 (Remove needless returns detected by clippy in the compiler)
 - rust-lang#130156 (Add test for S_OBJNAME & update test for LF_BUILDINFO cl and cmd)
 - rust-lang#130168 (maint: update docs for change_time ext and doc links)
 - rust-lang#130239 (miri: fix overflow detection for unsigned pointer offset)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#129103 (Don't warn empty branches unreachable for now)
 - rust-lang#129696 (update stdarch)
 - rust-lang#129835 (enable const-float-classify test, and test_next_up/down on 32bit x86)
 - rust-lang#130077 (Fix linking error when compiling for 32-bit watchOS)
 - rust-lang#130114 (Remove needless returns detected by clippy in the compiler)
 - rust-lang#130168 (maint: update docs for change_time ext and doc links)
 - rust-lang#130228 (notify Miri when intrinsics are changed)
 - rust-lang#130239 (miri: fix overflow detection for unsigned pointer offset)
 - rust-lang#130244 (Use the same span for attributes and Try expansion of ?)
 - rust-lang#130248 (Limit `libc::link` usage to `nto70` target only, not NTO OS)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a31a8fe into rust-lang:master Sep 12, 2024
12 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 12, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2024
Rollup merge of rust-lang#130114 - eduardosm:needless-returns, r=compiler-errors

Remove needless returns detected by clippy in the compiler
@eduardosm eduardosm deleted the needless-returns branch September 12, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants