mystor / rust-cpp

Embed C++ directly inside your rust code!
Apache License 2.0
795 stars 44 forks source link

Clean up clippy warnings #93

Closed ratijas closed 3 years ago

ratijas commented 3 years ago

Supersedes #81.

Now cargo clippy produces no warnings at all. I had to #[allow] that suspicious_map though, because there seems to be no pretty workaround.

ratijas commented 3 years ago

Requesting a review from sir @ogoffart, because he also has commit access here. r?

ratijas commented 3 years ago

Thank you, looks good.

I just would avoid doing the changes in the test.

I honestly didn't pay much attention to the meaning of surrounding code, but changes themselves looked reasonable, and tests are still passing. Is there some particular issue which I'm not aware of?

ogoffart commented 3 years ago

Well, the point of a test is to make sure that some construct works. For example, we need to test that the return statement do the right thing, even if it is not the best style.

ratijas commented 3 years ago

Ah, I see. Maybe a separate dedicated test for a syntax/parser then? Because this affected test is clearly for floating point values.

ratijas commented 3 years ago

I looked into implementation and cargo expand output, and I think now I understand your concern. For the reference, this is how macro rewrites the affected ptrCallback function:

#[allow(non_snake_case)]
#[allow(unused_unsafe)]
#[doc(hidden)]
#[no_mangle]
pub unsafe extern "C" fn ptrCallback(
    ptr: *const *mut u32,
    a: *const u32,
    rt: *mut *mut u32,
) -> *mut *mut u32 {
    let ptr: *mut u32 = unsafe { ptr.read() };
    let a: u32 = unsafe { a.read() };
    {
        #[allow(unused_mut)]
        let mut lambda = || {
            unsafe { *ptr += a };
            return ptr;
        };
        unsafe { ::core::ptr::write(rt, lambda()) };
    }
    ::core::mem::forget(ptr);
    ::core::mem::forget(a);
    rt
}

I tried to add #[allow(clippy::needless_return)] only to the return statement, but it does not work. I also tried #![allow] thing, but compiler thinks differently:

error[E0658]: attributes on expressions are experimental
  --> test/src/lib.rs:50:13
   |
50 |             #![allow(clippy::needless_return)]
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #15701 <https://github.com/rust-lang/rust/issues/15701> for more information

I think it might worth adding attribute syntax support inside rust! pseudo-macro. Meanwhile, I just added a test case which explicitly checks return statements and does so in a way that makes clippy happy.

ratijas commented 3 years ago

Ping? @mystor or @ogoffart

ogoffart commented 3 years ago

I still think the test shouldn't have been changed... but that's good enough. Thanks

ratijas commented 3 years ago

AppVeyor is at it again? Or is it just Microsoft Visual Studio 12.0 is far behind what's currently officially supported by Rust?

I still think the test shouldn't have been changed... but that's good enough. Thanks

Well, if anyone removes the closure from generated code, the new test would fail, and its name would give kinda helpful insight. By the way, I think it would be better to rewrite it as a standalone function instead, so it can support attributes in rust! macro (which is what I tried to silence clippy warning to no avail).