Skip to content

Suggest Cstr::count_bytes in strlen_on_c_strings #16323

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.

Sign up for GitHub

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

Jump to bottom
Open
Tigls wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Suggest Cstr::count_bytes in strlen_on_c_strings #16323

Tigls wants to merge 7 commits into from
+130 −40

Conversation

@Tigls
Copy link

@Tigls Tigls commented Jan 1, 2026 •
edited by rustbot
Loading

Closes #16308

changelog: [strlen_on_c_strings]: changes suggestion to use CStr::count_bytes()

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 1, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 1, 2026

r? @dswij

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@dswij
Copy link
Member

dswij commented Jan 1, 2026

@Tigls CI is failing on fmt check

@dswij dswij added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 1, 2026
samueltardieu
samueltardieu requested changes Jan 1, 2026
View reviewed changes
Copy link
Member

@samueltardieu samueltardieu left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

Could you please add tests below/at the considered MSRV, and inside/outside const contexts?

View changes since this review

clippy_lints/src/strlen_on_c_strings.rs Outdated
&& let ExprKind::MethodCall(path, self_arg, [], _) = recv.kind
&& !recv.span.from_expansion()
&& path.ident.name == sym::as_ptr
&& self.msrv.meets(cx, msrvs::CONST_BLOCKS)
Copy link
Member

@samueltardieu samueltardieu Jan 1, 2026

Choose a reason for hiding this comment

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

What is the link between .count_bytes() and CONST_BLOCKS?

Also:

  • If the MSRV isn't met, the previous suggestion should still apply.
  • Different MSRV apply in and outside a const context.

Copy link
Author

@Tigls Tigls Jan 2, 2026

Choose a reason for hiding this comment

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

yeah, I didn't know I can add new names to MSRV enum, thought they are kind of fixed code names for versions.

But makes sense, all fixed now.

Copy link
Member

@samueltardieu samueltardieu Jan 2, 2026

Choose a reason for hiding this comment

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

Don't forget to add tests for all combinations of MSRV/const contexts. You can look at how it's done in other tests by looking for the #[clippy::msrv] attribute.

Copy link
Author

@Tigls Tigls Jan 3, 2026

Choose a reason for hiding this comment

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

Looks like I can't call the libc::strlen in the const context. Since it is an FFI function, which is linked at runtime, it just won't compile. Does it mean I can safely remove the msrv branch for const context and keep only the branch for non-const contex, since the user won't be able to compile it anyway?

Copy link
Author

@Tigls Tigls Jan 8, 2026

Choose a reason for hiding this comment

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

@samueltardieu let me know if you had time to have a look and close this

@rustbot
Copy link
Collaborator

rustbot commented Jan 1, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@Tigls
Copy link
Author

Tigls commented Jan 5, 2026

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samueltardieu samueltardieu samueltardieu requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@dswij dswij

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Suggest Cstr::count_bytes in strlen_on_c_strings

4 participants

@Tigls @rustbot @dswij @samueltardieu