Skip to content

Conversation

kkysen
Copy link
Contributor

@kkysen kkysen commented Jun 26, 2022

I fixed some of the remaining clippy warnings (from #460) that needed slightly more complex refactors, which I want to have reviewed to make sure they're semantics-preserving.

kkysen added 7 commits June 25, 2022 23:11
…l` warning by applying the automatic fix and boolean reduction.
…hain into a `match` with `_ if`s.

Also able to avoid some `.is_some()`s so the `match` is exhaustive.
…esize` and factoring out common logic from both `match` arms.
I also eliminated a `.clone()` this way and made the `.pop()` logic explicit.
@kkysen kkysen requested a review from fw-immunant June 26, 2022 06:56
@kkysen
Copy link
Contributor Author

kkysen commented Jun 26, 2022

There are also 2 remaining clippy warnings that I had questions about:

  • clippy::duplicate_underscore_argument:

pub fn free(mir_loc: MirLocId, ptr: usize, _ptr: ()) {
TX.send(Event {
mir_loc,
kind: EventKind::Free { ptr },
})
.unwrap();
}

warning: `ptr` already exists, having another argument having almost the same name makes code comprehension and documentation more difficult
  --> analysis/runtime/src/handlers.rs:15:32
   |
15 | pub fn free(mir_loc: MirLocId, ptr: usize, _ptr: ()) {
   |                                ^^^
   |
   = note: `#[warn(clippy::duplicate_underscore_argument)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_underscore_argument

@rinon, do you know why there are 2 ptr arguments are why the second one is ignored and has a () type? What is it there for and why is it named ptr, too?

  • clippy::if_same_then_else:

// Only add linkage attributes if the function is `extern`
let mut mk_ = if is_main {
mk()
} else if is_global && !is_inline {
mk_linkage(false, new_name, name).extern_("C").pub_()
} else if is_inline && is_extern && !attrs.contains(&c_ast::Attribute::GnuInline) {
// c99 extern inline functions should be pub, but not gnu_inline attributed
// extern inlines, which become subject to their gnu89 visibility (private)
mk_linkage(false, new_name, name).extern_("C").pub_()
} else if self.cur_file.borrow().is_some() {
mk().extern_("C").pub_()
} else {
mk().extern_("C")
};

error: this `if` has identical blocks
    --> c2rust-transpile/src/translator/mod.rs:2377:51
     |
2377 |                   } else if is_global && !is_inline {
     |  ___________________________________________________^
2378 | |                     mk_linkage(false, new_name, name).extern_("C").pub_()
2379 | |                 } else if is_inline && is_extern && !attrs.contains(&c_ast::Attribute::GnuInline) {
     | |_________________^
     |
     = note: `#[deny(clippy::if_same_then_else)]` on by default
note: same as this
    --> c2rust-transpile/src/translator/mod.rs:2379:99
     |
2379 |                   } else if is_inline && is_extern && !attrs.contains(&c_ast::Attribute::GnuInline) {
     |  ___________________________________________________________________________________________________^
2380 | |                     // c99 extern inline functions should be pub, but not gnu_inline attributed
2381 | |                     // extern inlines, which become subject to their gnu89 visibility (private)
2382 | |
2383 | |                     mk_linkage(false, new_name, name).extern_("C").pub_()
2384 | |                 } else if self.cur_file.borrow().is_some() {
     | |_________________^
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else

@rinon, do you know if these two branches are meant to be identical, and if so, can they just be merged into one? I wasn't sure if they were supposed to remain separate, especially with a comment that's different in one branch.

@kkysen kkysen merged commit 9be4a8d into master Jun 28, 2022
@kkysen kkysen mentioned this pull request Jun 30, 2022
9 tasks
@kkysen kkysen deleted the kkysen/remaining-clippy-warnings branch July 9, 2022 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants