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

growth-ring: Add remaining lint checks #397

Merged
merged 2 commits into from
Dec 1, 2023
Merged

Conversation

rkuris
Copy link
Collaborator

@rkuris rkuris commented Nov 30, 2023

Removed a couple of index dereferences and unwraps.

Unfortunately, there's one spot where it isn't easy to add the lint check, so there is a block that disables it. It's still a lot better having it in most of the project and not in this block. (search for TODO: not allowed for the spot).

Rewriting the loop to be more idiomatic is probably the right move here, but that's outside the scope of this fix.

Removed a couple of index dereferences and unwraps.

Unfortunately, there's one spot where it isn't easy to add the lint
check, so there is a block that disables it. It's still a lot better
having it in most of the project and not in this block.
(search for TODO: not allowed for the spot).

Rewriting the loop to be more idiomatic is probably the right move here,
but that's outside the scope of this fix.
@@ -35,4 +35,6 @@ crate-type = ["dylib", "rlib", "staticlib"]

[lints.clippy]
unwrap_used = "warn"
indexing_slicing = "warn"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe do not enable warning on indexing_slicing until we have fix most of them, as there are a bunch of places we need to explicitly disable them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea here is that we want to prevent future code from doing index slicing, so this is a stopgap until we can get rid of it.

The plan is to do the same thing with firewood, which has a lot more of these. I have a tool to do that. I think having them marked as such means someone will be able to easily find and fix them later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, not feeling strong about this.

Copy link
Contributor

@richardpringle richardpringle left a comment

Choose a reason for hiding this comment

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

Index slicing is a little less trivial than disallowing unwrap.

There are times when you call a method to get a slice of a specific length and as long as your index is smaller than said length, you're fine indexing into the slice. It's these cases where the compiler wouldn't even do a bounds check.

Do we want to keep this type of behaviour and add the lint-ignore attribute?

It's possible that this never comes up. I can already think of places I do this but could do it without indexing. But in the case where it does, it would suck to make a function return a Result if it didn't need to.

I'm approving the PR because I agree with the change in general, but I would like to have a strategy in case the aforementioned scenario does come up.

@rkuris
Copy link
Collaborator Author

rkuris commented Nov 30, 2023

Index slicing is a little less trivial than disallowing unwrap.

Please have a look at the other 2 proposed lint changes going forward that were approved in #356 to ensure we're all in agreement on direction before I continue with these changes.

@rkuris rkuris merged commit 3f71366 into main Dec 1, 2023
5 checks passed
@rkuris rkuris deleted the rkuris/gr-remaining-lints branch December 1, 2023 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants