-
Notifications
You must be signed in to change notification settings - Fork 18
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
Embark standard lints v0.3 #60
Conversation
Suggestions:
|
|
cool, seen it used in a few crates but haven't tested that much myself with it. but sounds like a great idea!
ah yes that one could be interesting! we'll have to check a bit how we use it in different systems, but a new systems that is WIP can indeed opt into it.
that one is is definitely nice in concept, used it manually a few times. But it has multiple blocking issues: https://github.com/rust-lang/rust-clippy/issues?q=is%3Aissue+is%3Aopen+unnecessary_wraps, will link the key ones in #59 |
(positing here instead because that seems to be the better place) Personally I really don't like |
that lint is indeed a bit messy with some of those suggestions, there are smaller and simpler fixes it does that are great, but a lot of math expressions do get remangled quite a bit for performance which can be a bit too much. we do have it disabled on some crates (including shaders). We could potentially remove it, though many of the other smaller suggestions are good. |
Yeah, I really wish that it was broken up into one or two smaller lints since most of the other ones are a pure win both from a performance and a readability perspective. |
Agreed that would be nice, you could try filing a clippy GitHub issue suggestion on it? Looks pretty common to split up / refine lints that way |
Narrowed down a concrete working set of additional lints and tested both in Ark and in Rust GPU (https://github.com/EmbarkStudios/ark/pull/3302) and seems to work well and should get feedback from that tomorrow. A bunch of lints didn't make it, but that is fine, there will be later future versions and good not to do giant lists of lints as increases the risk and integration challenge with them. Anyone strongly object to any of the new lints? (not for some instances or specific crate, as that can be |
@MarijnS95 feel free to check this out as well! |
@repi Thanks! Looks good on our side except for |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, all very nice suggestions, I'd love to see needless_pass_by_value
added soon too, as its a trap we very often fall into as a team less experienced with rust!
@iffyio would you have time to see if we trip any of these additions in the src repo ?
Glad to hear it, and yes that lint is really good also, it does have some a bit tricky workarounds in some cases I've found in Ark, but have mostly gotten it working with it so think we can include it in the next version. |
Ah interesting, and good feedback! I could see that happening for some cases, if it is just for a pretty specific case with a very big value and you don't want to refactor it, then likely fine to just |
Joke's on us, we found out |
Current plan for v0.3
map_err_ignore
- prevents loosing error context and works well in Ark, easy to ignore when neededmatch_same_arms
- seems to work well and simplifies matching code to reduce complexity, with some exceptions.large_types_passed_by_value
- seems like a good idea, could catch some things should be boxed. no warns in Ark.string_add
- cleaner & more explicit, only 1 easy fixed warn in Ark.string_add_assign
- cleaner & more explicit, only 1 easy fixed warn in Ark.explicit_iter_loop
- we already useexplicit_into_iter_loop
in v0.2, should enable or disable both.unimplemented
- good for stubbing out WIP code but want to avoid onmain
our crates & systems are live at headThe
match_same_arms
lint is definitely the most intrusive change of these, and may need some opt outs in a few cases when one doesn't want to re-order code, but have been quite favored still.Under consideration, but currently not in v0.3
Some discussion in the PR but think these require more testing or follow up.
needless_pass_by_value
- helped us catch a lot of unnecessary: Vec<T>
arguments and replace them with: &[T]
, saving a lot of calls to.clone()
(very common trap for rust beginners to fall into). quite intrusive change for lots of crates though so safer to do separtelydoc_markdown
- has a 4y old outstanding issue on acronyms that it wants to force codeformatting backticks on which is quite messy and requires a lot of manual disabling (or aclippy.toml
config): doc_markdown should ignore things which just contain acronyms rust-lang/rust-clippy#1136pub_enum_variant_names
- pretty common for this one to trigger in valid/good cases, so is a bit hit & miss.suboptimal_flops
- rearranges code quite significantly withmul_add
Will leave this as a draft for a while and feel free to discuss & suggest additional additions or removals