Clippy related fixes #469

tempdragon · 2024-03-11T09:18:55Z

I don't know if you are interested in merging this half-generated(if not fully) commit. If not, feel free to close it. Everything is generated if not mentioned.
I do apologize for a likely delay in answering the reviews.
I renamed the methods from_low_high_rvalues and from_low_high in tempdragon/rustc_codegen_gcc@a7d39b8
All life time related modifications were done by using cargo clippy --fix --lib -p rustc_codegen_gcc --allow-dirtyxs, as is done in tempdragon/rustc_codegen_gcc@ad97b8c. However, since I am not an expert in lifetime, I can't guarantee its correctness(But it passed the workflow anyway.)
In tempdragon/rustc_codegen_gcc@d2cda90, the shift was removed for use of i, but the shift variable still looks weird here.
I used a clone() to solve pattern_type_mismatch lint in tempdragon/rustc_codegen_gcc@8d4d878, but it looks less efficient. I should apologize because I do doubt if there is a better way to fix this here.

Probably gonna solve #457.

1. Fix Pattern Type Mismatch by Adding deref's 2. Move commented `else if` to previous block in `intrinsic.rs`

1. Use `clone_from` in place of `clone()` in `builder.rs` 2. Change `&name` to `name.clone()` in `debuginfo.rs`(Is this really efficient? But I can't find other workarounds.)

cmd: `cargo clippy --fix --lib -p rustc_codegen_gcc --allow-dirtyxs`

This looks like an (inverted) exclusive-or but I still leave it as it is in clippy.

Changed for clippy naming convention requirement: ``` warning: methods called `from_*` usually take no `self` --> src/int.rs:996:22 | 996 | fn from_low_high(&self, typ: Type<'gcc>, low: i64, high: i64) -> RValue<'gcc> { | ^^^^^ | = help: consider choosing a less ambiguous name = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention = note: `#[warn(clippy::wrong_self_convention)]` on by default ```

antoyo

Thanks! The clone() should not be necessary.

antoyo · 2024-03-15T20:30:58Z

src/builder.rs

-    bx: &mut Builder<'a, 'gcc, 'tcx>,
-    rvalue: RValue<'gcc>,
-) -> RValue<'gcc> {
+fn set_rvalue_location<'gcc>(bx: &mut Builder<'_, 'gcc, '_>, rvalue: RValue<'gcc>) -> RValue<'gcc> {


Not sure I like this change (since I fear this might make the lifetime error messages harder to understand), but I'm not sure if there's a way to disable this lint in a way that clippy will still warn about lifetimes that could be removed on simple references.

Opened an issue.

Currently we should simply close the lint, I guess.

I will revert the whole tempdragon@ad97b8c.

Reverted in tempdragon/rustc_codegen_gcc@390f906, adding #![allow(clippy::needless_lifetimes)] to allow this in clippy.

It seems this specific function (at least) was not reverted.

src/debuginfo.rs

src/declare.rs

src/int.rs

src/intrinsic/mod.rs

antoyo · 2024-03-15T20:54:29Z

src/intrinsic/mod.rs

+                    /*else if use_integer_compare {
+                        let integer_ty = self.type_ix(layout.size.bits()); // FIXME(antoyo): LLVM creates an integer of 96 bits for [i32; 3], but gcc doesn't support this, so it creates an integer of 128 bits.
+                        let ptr_ty = self.type_ptr_to(integer_ty);
+                        let a_ptr = self.bitcast(a, ptr_ty);
+                        let a_val = self.load(integer_ty, a_ptr, layout.align.abi);
+                        let b_ptr = self.bitcast(b, ptr_ty);
+                        let b_val = self.load(integer_ty, b_ptr, layout.align.abi);
+                        self.icmp(IntPredicate::IntEQ, a_val, b_val)
+                    }*/
+                } else {


I believe this is not correct. Please revert to the original code, but put the else on the same line as the end of the comment, like this:

}*/ else {

Sadly, the rustfmt doesn't seem to allow this.

It automatically inserts a newline and then cause the following warning:

warning: this is an `else {..}` but the formatting might hide it --> src/intrinsic/mod.rs:311:18 | 311 | ... } | ________^ 312 | | ... /*else if use_integer_compare { 313 | | ... let integer_ty = self.type_ix(layout.size.bits()); // FIXME(antoyo): LLVM creates... 314 | | ... let ptr_ty = self.type_ptr_to(integer_ty); ... | 320 | | ... }*/ 321 | | ... else { | |___________^ | = note: to remove this lint, remove the `else` or remove the new line between `else` and `{..}` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_else_formatting = note: `#[warn(clippy::suspicious_else_formatting)]` on by default

I don't currently know if there is a way that fits both your requirements and rustfmt and clippy.

Should I try to have clippy ignore this or should I handle this in other ways?

src/intrinsic/mod.rs

antoyo · 2024-03-15T21:22:31Z

Also, if you want, please add cargo clippy --all-targets -- -D warnings somewhere in one CI workflow.

1. Put the `else if` comment in intrinsic/mod.rs away 2. Add TODO to debuginfo.rs in `make_mir_scope()`

This reverts commit ad97b8c.

This reverts commit f6cab2c.

Done incrementally to avoid potential failure.

tempdragon · 2024-03-16T04:42:33Z

Also, if you want, please add cargo clippy --all-targets -- -D warnings somewhere in one CI workflow.

Added in 4e4efb9

src/intrinsic/mod.rs

src/attributes.rs

src/consts.rs

src/intrinsic/mod.rs

antoyo

This is good to me.

@tempdragon: Please tell me if you wanted to do any other changes or if I can merge this PR.

tempdragon · 2024-03-16T14:33:22Z

Look good to me. Thanks for your review!

antoyo · 2024-03-16T14:34:30Z

Thanks for your contribution!

tempdragon added 9 commits March 11, 2024 10:24

fix(fmt/style): Remove unncessary clones, into's and deref's

6f76488

fix(fmt/style): Further apply Clippy suggestions manually

9ea3c19

1. Fix Pattern Type Mismatch by Adding deref's 2. Move commented `else if` to previous block in `intrinsic.rs`

fix(declare.rs): Clone name when buiding without the master feat.

c6b7558

fix(clippy): Clone-related clippy workarounds

8d4d878

1. Use `clone_from` in place of `clone()` in `builder.rs` 2. Change `&name` to `name.clone()` in `debuginfo.rs`(Is this really efficient? But I can't find other workarounds.)

fix(liftime): Gen. by cargo clippy

ad97b8c

cmd: `cargo clippy --fix --lib -p rustc_codegen_gcc --allow-dirtyxs`

fix(fmt): Try to comply to the format requirement

091de55

fix(clippy): redundant variables in simd.rs

d2cda90

fix(fmt): Rewrite a condition according to clippy

3fd9db3

This looks like an (inverted) exclusive-or but I still leave it as it is in clippy.

antoyo reviewed Mar 15, 2024

View reviewed changes

tempdragon added 8 commits March 16, 2024 09:43

fix(pattern_type_mismatch)): Fix mismatch with ref/deref

817d2f2

fix(comments): Add some info and revert else if

878f572

1. Put the `else if` comment in intrinsic/mod.rs away 2. Add TODO to debuginfo.rs in `make_mir_scope()`

revert(lifetime): Add "needless" lifetime(s) and allow it in clippy

390f906

This reverts commit ad97b8c.

feat(CI): Add clippy check to workflow.

f6cab2c

feat(clippy): Suppress lint suspicious_else_formatting temporarily

653118e

Revert "feat(CI): Add clippy check to workflow."

225a32f

This reverts commit f6cab2c.

feat(ci): Install clippy in ci.yml(But no testing)

89acb10

Done incrementally to avoid potential failure.

feat(ci): Add clippy check for both master and non-master

4e4efb9

antoyo reviewed Mar 16, 2024

View reviewed changes

tempdragon added 2 commits March 16, 2024 21:16

fix(intrinsic/mod.rs): Update comments

3b01fba

fix(lifetime): Add lifetimes back.

0a49351

tempdragon force-pushed the master branch from 998aad9 to 0a49351 Compare March 16, 2024 13:30

antoyo approved these changes Mar 16, 2024

View reviewed changes

antoyo merged commit 7ff5d39 into rust-lang:master Mar 16, 2024
34 checks passed

antoyo mentioned this pull request Mar 16, 2024

Enable Rustfmt & Clippy for cg_gcc #457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clippy related fixes #469

Clippy related fixes #469

tempdragon commented Mar 11, 2024 •

edited

Loading

antoyo left a comment

antoyo Mar 15, 2024

antoyo Mar 15, 2024

tempdragon Mar 16, 2024

tempdragon Mar 16, 2024

tempdragon Mar 16, 2024

antoyo Mar 16, 2024

antoyo Mar 15, 2024

tempdragon Mar 16, 2024

tempdragon Mar 16, 2024

tempdragon Mar 16, 2024

tempdragon Mar 16, 2024

antoyo commented Mar 15, 2024

tempdragon commented Mar 16, 2024 •

edited

Loading

antoyo left a comment

tempdragon commented Mar 16, 2024

antoyo commented Mar 16, 2024

Clippy related fixes #469

Clippy related fixes #469

Conversation

tempdragon commented Mar 11, 2024 • edited Loading

antoyo left a 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

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

Choose a reason for hiding this comment

antoyo commented Mar 15, 2024

tempdragon commented Mar 16, 2024 • edited Loading

antoyo left a comment

Choose a reason for hiding this comment

tempdragon commented Mar 16, 2024

antoyo commented Mar 16, 2024

tempdragon commented Mar 11, 2024 •

edited

Loading

tempdragon commented Mar 16, 2024 •

edited

Loading