Rust-clippy: identity_op warns for bitflag constant

[konstin]

pyo3 has to map certain constants and flags from the python headers, which includes the code below:

// Flag bits for printing:
pub const Py_PRINT_RAW: c_int = 1; // No string quotes etc.

// PyBufferProcs contains bf_getcharbuffer
pub const Py_TPFLAGS_HAVE_GETCHARBUFFER: c_long = (1 << 0);

// PySequenceMethods contains sq_contains
pub const Py_TPFLAGS_HAVE_SEQUENCE_IN: c_long = (1 << 1);

// PySequenceMethods and PyNumberMethods contain in-place operators
pub const Py_TPFLAGS_HAVE_INPLACEOPS: c_long = (1 << 3);

// PyNumberMethods do their own coercion
pub const Py_TPFLAGS_CHECKTYPES: c_long = (1 << 4);

This code raises a warning:

warning: the operation is ineffective. Consider reducing it to `1`
   --> src/ffi2/object.rs:676:51
    |
676 | pub const Py_TPFLAGS_HAVE_GETCHARBUFFER: c_long = (1 << 0);
    |                                                   ^^^^^^^^
    |
    = note: #[warn(clippy::identity_op)] on by default

The explanation says:

This code can be removed without changing the meaning. So it just obscures what’s going on. Delete it mercilessly.

IMHO (1 << 0) is not obscuring the code, but relevant for pointing out that this constant is the first entry in a bitfield. Therefore this is imho a false positive in clippy.

cargo clippy -V: clippy 0.0.212 (a20599a 2018-11-01)

[flip1995]

I'm not sure if this is really a false positive, since 1 == (1 << 0) and therefore _ << 0 is indeed an identity op. IMHO this is a TP and the bitfield creation is a special case, where it is pretty opinionated if (1 << 0) is better for pointing out, that this is a element of the bitfield or if just 1 is sufficient.

I definitely see your point, but I think this should be handled by allowing this lint in that case.

[konstin]

It's for sure an identity op, but I'd argue that it's not bad style or erroneous, but an important note for what that constant does. It would also be inconsistent to use x << 0 everywhere else but not on the first element.

[flip1995]

I would suggest to allow (_ << 0) in constant definitions. But I think generally not linting it would be a bad idea.

[konstin]

I agree

[oli-obk]

so... generally constant << 0 or just in constant contexts?

[konstin]

Good question. All seems like a good choice because extern_c_function((1 << 0) | (1 << 2)) would be reasonble, otoh you could argue you should define constants for that.

[Disasm]

Two additional examples: here and here.
I think, at least 1 << 0 construction should be allowed in any context.

[khoek]

I've just run into this in several places writing code for an embedded target.

I agree, I think the pattern 1 << 0 verbatim is so common in bit manipulations (compared even to all variants of MASK << 0) that it would be worth it to allow.