Use Embark standard lints v0.3 #482
Conversation
|
Fixed CI |
| @@ -60,7 +59,9 @@ impl<'tcx> CodegenCx<'tcx> { | |||
| SpirvType::Integer(32, true) => self | |||
| .builder | |||
| .def_constant(SpirvConst::U32(ty, val as i64 as i32 as u32)), | |||
| SpirvType::Integer(64, true) => self.builder.def_constant(SpirvConst::U64(ty, val)), | |||
| SpirvType::Integer(64, false) | SpirvType::Integer(64, true) => { | |||
There was a problem hiding this comment.
This can be collapsed further into SpirvType::Integer(64, _) - note that the 32-bit case can also be collapsed, if you replace val as i64 as i32 as u32 with val as u32 (they're equivalent, no order of truncations is different from any other, since you're chopping off bits - {sign,zero}-extension can only happen when you're widening the type).
There was a problem hiding this comment.
sounds like a good idea
| SpirvConst::Bool(_, false) => 0, | ||
| SpirvConst::Bool(_, true) => 0, | ||
| SpirvConst::Bool(_, false) | SpirvConst::Bool(_, true) => 0, |
There was a problem hiding this comment.
This looks like a mistake, should be SpirvConst::Bool(_, v) => v as u8.
There was a problem hiding this comment.
suspected that as well, can you apply that change?
| Self::Vector { element, count } => { | ||
| cx.lookup_type(element).sizeof(cx)? * count.next_power_of_two() as u64 | ||
| } | ||
| Self::Array { element, count } => { | ||
| cx.lookup_type(element).sizeof(cx)? * cx.builder.lookup_const_u64(count).unwrap() | ||
| } | ||
| Self::RuntimeArray { .. } => return None, | ||
| Self::Pointer { .. } => cx.tcx.data_layout.pointer_size, | ||
| Self::Function { .. } => cx.tcx.data_layout.pointer_size, |
There was a problem hiding this comment.
This one is wrong I'm pretty sure, a SPIR-V function is not a function pointer, and should return None here - assuming that still can compile examples and tests, ofc.
There was a problem hiding this comment.
nice that the lint caught it, it does read better and easier to spot bugs when same arms are collapsed!
can you do an issue or fix it in a separate PR? not sure if good idea to fix and change behavior here in the lint PR. or can you commit directly into this PR? I would be more confident a compiler engineer does these behavior changes than me enabling lints :)
| clippy::todo, // still lots to implement :) | ||
| clippy::todo // still lots to implement :) |
There was a problem hiding this comment.
Huh, we use todo!()? I remember trying to use unimplemented!() and getting an error from a lint.
There was a problem hiding this comment.
yup plenty of todo! in the crate, and only like 1 unimplemented!. I guess they both do a very similar
| SpirvType::Bool => TypeKind::Integer, // thanks llvm | ||
| SpirvType::Integer(_, _) => TypeKind::Integer, |
There was a problem hiding this comment.
This looks like a lint bug, it should suggest collapsing these two cases, but somehow it finds the other ones down below instead?
There was a problem hiding this comment.
It did suggest collapsing them, that's why I had to move it down to the integer arm, but formatted it a bit differently to keep the comment.
Would indeed be good if the lint took comments into consideration, as we talked about on discord. but this is the best I could get it to do without that.
any ideas on how to improve it here in the code?
|
This PR now needs a rebase to be merged. |
|
@XAMPPRocky can you take it over? |
Co-Authored-By: Johan Andersson <[email protected]>
XAMPPRocky
force-pushed
the
embark-lints-v0.3
branch
from
914cf2a to
7cba276
Compare
Fixes up our code to support the additional lints in the draft v0.3 of our standard set: EmbarkStudios/rust-ecosystem#60. This is not final yet, but getting close there. Probably be quite a while until the next version after this, now we have most of the main lints we've been testing with.
The main change was with the
match_same_armslint, many match cases worked nicely and got merged together which do find easier to read, but some would require larger ordering changes & merges which looked like it could reduce readability in the complex backend code so left it with `allow´ in those few cases.Leaving this as draft until EmbarkStudios/rust-ecosystem#60 is merged and v0.3 finalized. If you have any issues with any of these changes/lints, please do comment here and/or in the PR if we should not enable one of the lints.