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 workaround false positive clippy lint reversed_empty_ranges #1279

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ModProg
Copy link

@ModProg ModProg commented Apr 23, 2023

As the rust-clippy issue rust-lang/rust-clippy#5808 doesn't seam to make much progress I propose to just allow the lint for s![] macro invocations for now.

The let = ... is necessary, because attributes are not allowed on expressions
currently (rust-lang/rust#15701).

@ModProg
Copy link
Author

ModProg commented Apr 30, 2023

@bluss removed the depricated zip

@bluss
Copy link
Member

bluss commented Apr 30, 2023

Can we hold off on removing the deprecated zip until it can be replaced by std::iter::zip? That's after all the whole point of it, to use the function version :slightly_smiling_face:

(Function zip is the best 🙂 )

@ModProg
Copy link
Author

ModProg commented Apr 30, 2023

Yeah no problem, only did so because the ci was failing because of it.

src/slice.rs Outdated
]
{
#[allow(clippy::reversed_empty_ranges)]
let slice = $crate::s![@parse
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you try using #![allow(clippy::reversed_empty_ranges)] to allow this for the block and thereby avoiding the temporary binding? (Tail expressions do influence lifetime extension of temporaries and hence I would like to avoid changing this if it can be avoided.)

Copy link
Author

Choose a reason for hiding this comment

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

attributes on expressions are experimental

Wouldn't work, as it needs a statement to apply the attribute

Copy link
Collaborator

@adamreichold adamreichold May 23, 2023

Choose a reason for hiding this comment

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

Note the exclamation mark. This is similar to the form used at the beginning of a module to apply a directive to the whole module. So this would apply to everything inside the block, which in this case is just a single expression.

Copy link
Author

@ModProg ModProg May 23, 2023

Choose a reason for hiding this comment

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

($($t:tt)*) => {
    {
        #![allow(clippy::reversed_empty_ranges)]
        $crate::s![@parse
            ::core::marker::PhantomData::<$crate::Ix0>,
            ::core::marker::PhantomData::<$crate::Ix0>,
            []
            $($t)*
        ]
    }
};
error[E0658]: attributes on expressions are experimental
   --> /mnt/data-ssd/modprog/Development/Rust/ndarray/src/slice.rs:871:13
    |
775 | macro_rules! s(
    | -------------- in this expansion of `s!`
...
871 |             #![allow(clippy::reversed_empty_ranges)]
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
   ::: tests/array.rs:202:16
    |
202 |     let info = s![1.., 1, NewAxis, ..;2];
    |                ------------------------- in this macro invocation
    |
    = note: see issue #15701 <https://github.com/rust-lang/rust/issues/15701> for more information
    = help: add `#![feature(stmt_expr_attributes)]` to the crate attributes to enable

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, so this did work. Sorry for that.

Could you give the diff

diff --git a/src/slice.rs b/src/slice.rs
index 0146d6db..53763ae2 100644
--- a/src/slice.rs
+++ b/src/slice.rs
@@ -779,7 +779,7 @@ macro_rules! s(
             r => {
                 let in_dim = $crate::SliceNextDim::next_in_dim(&r, $in_dim);
                 let out_dim = $crate::SliceNextDim::next_out_dim(&r, $out_dim);
-                #[allow(unsafe_code)]
+                #[allow(unsafe_code, clippy::reversed_empty_ranges)]
                 unsafe {
                     $crate::SliceInfo::new_unchecked(
                         [$($stack)* $crate::s!(@convert r, $s)],
@@ -796,7 +796,7 @@ macro_rules! s(
             r => {
                 let in_dim = $crate::SliceNextDim::next_in_dim(&r, $in_dim);
                 let out_dim = $crate::SliceNextDim::next_out_dim(&r, $out_dim);
-                #[allow(unsafe_code)]
+                #[allow(unsafe_code, clippy::reversed_empty_ranges)]
                 unsafe {
                     $crate::SliceInfo::new_unchecked(
                         [$($stack)* $crate::s!(@convert r)],

a try? It does work for me without forcing the slice info into a local binding.

If it does, could use this approach and rebase this on the current master branch?

Copy link
Author

Choose a reason for hiding this comment

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

That didn't work, but I found a different solution, using two blocks to convince rust to allow an attribute on an expression.

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.

3 participants