Skip to content
This repository has been archived by the owner on Oct 21, 2022. It is now read-only.

Fix linting issues related to the simd macro #307

Merged
merged 2 commits into from
Jul 28, 2021
Merged

Fix linting issues related to the simd macro #307

merged 2 commits into from
Jul 28, 2021

Conversation

chevdor
Copy link
Contributor

@chevdor chevdor commented Jul 27, 2021

The simd! macro is mainly (133 times) used with 2 expressions. Sometimes (18 times) however, a 3rd is needed.

This PR does not fix all the clippy issues, the rest is addressed in #306 and #305.

Initially, simd! was defined as:

#[cfg(feature="simd")]
macro_rules! simd {
	($writer: expr, $byte: expr, $other:expr) => ({
		$writer.write(&[SIMD_PREFIX])?;
		VarUint32::from($byte).serialize($writer)?;
		$other;
	})
}

Linting the expansion of the macro results in grumbles in many cases similar to the one we can see below.

warning: statement with no effect
    --> src/elements/ops.rs:1711:3
     |
1711 |         $other;
     |         ^^^^^^^
...
2296 |             I32x4TruncUF32x4Sat => simd!(writer, I32X4_TRUNC_U_F32X4_SAT, ()),
     |                                    ------------------------------------------ in this macro invocation
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_effect
     = note: this warning originates in the macro `simd` (in Nightly builds, run with -Z macro-backtrace for more info)

We have 2 options:

  • disable the linting warnings
  • have an additionnal macro

This PR proposes the second option.
What do you think @pepyakin ?
If you agree with the option and have better naming ideas, please let me know.

@chevdor
Copy link
Contributor Author

chevdor commented Jul 28, 2021

Alternatively, we can also keep a single macro and use {} instead of ()

@athei
Copy link
Member

athei commented Jul 28, 2021

I am really in favor of just disabling the lint for the macro. The only bad thing that that this does is making the generated code less readable. Now we are changing the generating code to be less readable (more macros) in order to fix a non-issue.

If you feel strongly about this we could at least collapse that into one macro so we do not need to touch the calling side:

#[cfg(feature="simd")]
macro_rules! simd {
	($writer: expr, $byte: expr, ()) => ({
		$writer.write(&[SIMD_PREFIX])?;
		VarUint32::from($byte).serialize($writer)?;
	});
	($writer: expr, $byte: expr, $other:expr) => ({
		$writer.write(&[SIMD_PREFIX])?;
		VarUint32::from($byte).serialize($writer)?;
		$other;
	})
}

But this will also add duplication for the sake of just fixing a lint that only affects code that no one will look at.

@chevdor
Copy link
Contributor Author

chevdor commented Jul 28, 2021

@athei I am also not a big fan of the change as I did it initially and after thinking further I think the best option is to keep one single macro but pass {} and not () for the macro calls where we don`t need the 3rd expression.

That makes the linter happy, allows not sutting down any linting and actually makes sense this way IMO.

@athei
Copy link
Member

athei commented Jul 28, 2021

I think the best option is to keep one single macro but pass {} and not () for the macro calls where we don`t need the 3rd expression.

Sounds good.

@chevdor
Copy link
Contributor Author

chevdor commented Jul 28, 2021

ok, so let's not merge this PR yet, I will rewrite it.

@chevdor chevdor changed the title Split the simd macro into 2 Fix linting issues related to the simd macro Jul 28, 2021
@chevdor chevdor merged commit 6715774 into master Jul 28, 2021
@chevdor chevdor deleted the wk-simd branch July 28, 2021 13:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants