Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some clippy warnings #468

Merged
merged 2 commits into from Mar 1, 2019

Conversation

Projects
None yet
3 participants
@EPashkin
Copy link
Member

commented Feb 26, 2019

@@ -189,9 +189,7 @@ impl ToGlib for () {
type GlibType = ();

#[inline]
fn to_glib(&self) -> () {

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Feb 26, 2019

Member

Why the /*-> ()*/?

This comment has been minimized.

Copy link
@EPashkin

EPashkin Feb 26, 2019

Author Member

Because clippy not like returning unit both in declaration and body

This comment has been minimized.

Copy link
@EPashkin

EPashkin Feb 26, 2019

Author Member

to_glib usually return something so I decide just comment

This comment has been minimized.

Copy link
@sdroege

sdroege Feb 27, 2019

Member

Fine with me :)

This comment has been minimized.

Copy link
@sdroege

sdroege Feb 27, 2019

Member

Why do we need this impl though?

This comment has been minimized.

Copy link
@EPashkin

EPashkin Feb 27, 2019

Author Member

Examples compiles without it, so it will be removed

@sdroege

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Should probably also run clippy as part of CI for glib at least.

@@ -85,7 +85,7 @@ impl Error {
}

// backcompat shim
#[cfg_attr(feature = "cargo-clippy", allow(not_unsafe_ptr_arg_deref))]
#[cfg_attr(feature = "cargo-clippy", allow(clippy::not_unsafe_ptr_arg_deref))]

This comment has been minimized.

Copy link
@sdroege

sdroege Feb 27, 2019

Member

AFAIU this can be simply #[clippy::blablabla] now without the cfg_attr stuff?

This comment has been minimized.

Copy link
@EPashkin

EPashkin Feb 27, 2019

Author Member

IMHO 1.31.0 still don't support it, will check at evening.
Thanks for reminder

@@ -96,7 +96,7 @@ impl MainContext {
}
}

#[cfg_attr(feature = "cargo-clippy", allow(transmute_ptr_to_ref))]
#[cfg_attr(feature = "cargo-clippy", allow(clippy::transmute_ptr_to_ref))]
unsafe extern "C" fn trampoline<F: FnOnce() + 'static>(func: gpointer) -> gboolean {
let func: &mut Option<F> = transmute(func);

This comment has been minimized.

Copy link
@sdroege

sdroege Feb 27, 2019

Member

This could simply be a cast: let func: &mut Option<F> = &*(func as *mut Option<F>)

@@ -337,6 +337,7 @@ pub trait Cast: ObjectType {
/// Casts to `&T` unconditionally.
///
/// Panics if compiled with `debug_assertions` and the instance doesn't implement `T`.
#[cfg_attr(feature = "cargo-clippy", allow(clippy::transmute_ptr_to_ptr))]

This comment has been minimized.

Copy link
@sdroege

sdroege Feb 27, 2019

Member

Here we should be able to cast too then, or not?

This comment has been minimized.

Copy link
@EPashkin

EPashkin Feb 27, 2019

Author Member

Maybe yes, now not sure why I decide ignoring.

@@ -98,7 +98,7 @@ impl Drop for CallbackGuard {
}
}

#[cfg_attr(feature = "cargo-clippy", allow(transmute_ptr_to_ref))]
#[cfg_attr(feature = "cargo-clippy", allow(clippy::transmute_ptr_to_ref))]
unsafe extern "C" fn trampoline<F: FnMut() -> Continue + 'static>(func: gpointer) -> gboolean {
let func: &RefCell<F> = transmute(func);

This comment has been minimized.

Copy link
@sdroege

sdroege Feb 27, 2019

Member

Can be a simple cast

@@ -113,7 +113,7 @@ fn into_raw<F: FnMut() -> Continue + 'static>(func: F) -> gpointer {
Box::into_raw(func) as gpointer
}

#[cfg_attr(feature = "cargo-clippy", allow(transmute_ptr_to_ref))]
#[cfg_attr(feature = "cargo-clippy", allow(clippy::transmute_ptr_to_ref))]
unsafe extern "C" fn trampoline_child_watch<F: FnMut(Pid, i32) + 'static>(pid: glib_ffi::GPid, status: i32, func: gpointer) {
let func: &RefCell<F> = transmute(func);

This comment has been minimized.

Copy link
@sdroege

sdroege Feb 27, 2019

Member

Can be a simple cast

@@ -296,6 +296,7 @@ pub unsafe trait ObjectClassSubclassExt: Sized + 'static {
unsafe impl ObjectClassSubclassExt for ObjectClass {}

unsafe impl<T: ObjectSubclass> IsSubclassable<T> for ObjectClass {
#[cfg_attr(feature = "cargo-clippy", allow(clippy::cast_ref_to_mut))]
fn override_vfuncs(&mut self) {
unsafe {
let klass = &mut *(self as *const Self as *mut gobject_ffi::GObjectClass);

This comment has been minimized.

Copy link
@sdroege

sdroege Feb 27, 2019

Member

Why do we even cast to a *const Self? Just make this a *mut Self and it will all be fine :)

@@ -138,6 +138,7 @@ impl Value {
///
/// Returns `Some(&TypedValue<T>)` if the value carries a type corresponding
/// to `T` and `None` otherwise.
#[cfg_attr(feature = "cargo-clippy", allow(clippy::transmute_ptr_to_ptr))]

This comment has been minimized.

Copy link
@sdroege

sdroege Feb 27, 2019

Member

Should be possible to use a cast here and below?

@EPashkin EPashkin force-pushed the EPashkin:clippy branch from 874120d to eed62bb Feb 27, 2019

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

Updated

@EPashkin EPashkin force-pushed the EPashkin:clippy branch from eed62bb to 7b36024 Feb 27, 2019

@@ -85,7 +85,7 @@ impl Error {
}

// backcompat shim
#[cfg_attr(feature = "cargo-clippy", allow(not_unsafe_ptr_arg_deref))]
#[allow(clippy::not_unsafe_ptr_arg_deref)]

This comment has been minimized.

Copy link
@sdroege

sdroege Feb 28, 2019

Member

We should probably remove this function? It says backwards compatibility :)

This comment has been minimized.

Copy link
@EPashkin

EPashkin Feb 28, 2019

Author Member

Yes, it not used

@@ -82,6 +82,7 @@ impl Closure {
from_glib_none(closure)
}

#[allow(clippy::redundant_closure)]
pub fn invoke(&self, values: &[&ToValue]) -> Option<Value> {

This comment has been minimized.

Copy link
@sdroege

sdroege Feb 28, 2019

Member

What does this mean exactly, what redudant closure and why can't we fix that?

This comment has been minimized.

Copy link
@EPashkin

EPashkin Feb 28, 2019

Author Member

It wan't change .map(|v| v.to_value()) to function but if I change it, then will be error.

error[E0277]: the trait bound `dyn value::ToValue: value::SetValue` is not satisfied
  --> D:/eap/rust/0/glib\src\closure.rs:98:22
   |
98 |                 .map(ToValue::to_value)
   |                      ^^^^^^^^^^^^^^^^^ the trait `value::SetValue` is not implemented for `dyn value::ToValue`
   |
   = note: required because of the requirements on the impl of `value::SetValue` for `&dyn value::ToValue`
   = note: required because of the requirements on the impl of `value::ToValue` for `&dyn value::ToValue`
note: required by `value::ToValue::to_value`
  --> D:/eap/rust/0/glib\src\value.rs:594:5
   |
594|     fn to_value(&self) -> Value;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Same problem for second redundant_closure

error[E0277]: the trait bound `std::vec::Vec<std::string::String>: translate::ToGlibPtr<'_, _>` is not satisfied
   --> D:/eap/rust/0/glib\src\value.rs:871:46
    |
871 |         let ptr: *mut *mut c_char = this.map(ToGlibPtr::to_glib_full).unwrap_or(ptr::null_mut());
    |                                              ^^^^^^^^^^^^^^^^^^^^^^^ the trait `translate::ToGlibPtr<'_, _>` is not implemented for `std::vec::Vec<std::string::String>`
    |
note: required by `translate::ToGlibPtr::to_glib_full`
   --> D:/eap/rust/0/glib\src\translate.rs:259:5
    |
259 |     fn to_glib_full(&self) -> P {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0277]: the trait bound `std::vec::Vec<std::string::String>: translate::ToGlibPtr<'_, _>` is not satisfied
   --> D:/eap/rust/0/glib\src\value.rs:871:37
    |
871 |         let ptr: *mut *mut c_char = this.map(ToGlibPtr::to_glib_full).unwrap_or(ptr::null_mut());
    |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `translate::ToGlibPtr<'_, _>` is not implemented for `std::vec::Vec<std::string::String>`

This comment has been minimized.

Copy link
@sdroege

sdroege Feb 28, 2019

Member

Does this need a as_ref() maybe? But fine with me

This comment has been minimized.

Copy link
@EPashkin

EPashkin Feb 28, 2019

Author Member

Not works too.

This comment has been minimized.

Copy link
@sdroege

sdroege Feb 28, 2019

Member

🤷‍♂

@sdroege

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Looks good to me otherwise, thanks! Can we run clippy as part of CI now or are there still warnings left?

@EPashkin EPashkin force-pushed the EPashkin:clippy branch from 7b36024 to 167269b Feb 28, 2019

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

Added clippy check on travis

@sdroege

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Added clippy check on travis

Please use stable clippy

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

I prefer use last version, you against "beta" too ?

@sdroege

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

I prefer use last version, you against "beta" too ?

The problem is that nightly (and beta?) is often released without clippy or other tools because of breakages and that would then make the CI fail for no good reason. Otherwise I'd also prefer nightly to have the latest checks.

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

As beta is next stable so it must have clippy, I try change to it

@EPashkin EPashkin force-pushed the EPashkin:clippy branch from 167269b to 781a66a Feb 28, 2019

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

Maybe we need test both beta and nightly but change script that it don't fails on missing component?

@sdroege

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Maybe we need test both beta and nightly but change script that it don't fails on missing component?

I don't think that's worth it. clippy is not changing that often :) Let's merge this as is?

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

Yes, we can't fix failing nightly clippy, so lets merge current version if @GuillaumeGomez don't have better solution.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Like I said, I'm fine as is.

@sdroege sdroege merged commit 92b365b into gtk-rs:master Mar 1, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@EPashkin EPashkin deleted the EPashkin:clippy branch Mar 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.