Skip to content

Conversation

@ada4a
Copy link
Contributor

@ada4a ada4a commented Oct 25, 2025

changelog: none

@rustbot rustbot added needs-fcp S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 25, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 25, 2025

r? @Alexendoo

rustbot has assigned @Alexendoo.
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

@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

Comment on lines 3530 to 3536
pub fn is_allowed_vec_method(e: &Expr<'_>) -> bool {
if let ExprKind::MethodCall(path, _, [], _) = e.kind {
matches!(path.ident.name, sym::as_ptr | sym::is_empty | sym::len)
} else {
false
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't check that the method call actually is on a Vec, but I'm aware that would be redundant for the current callsites. Perhaps something more like more like Symbol -> bool would be fine, or it could be inlined

But if it's kept it needs a rename since which names are allowed doesn't make sense out of the original context

Copy link
Contributor Author

@ada4a ada4a Nov 2, 2025

Choose a reason for hiding this comment

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

Great point! I stripped it all the way down to a const array, which turned out to work rather nicely, even allowing to simplify the logic in ptr_arg.

I think the new name is pretty descriptive? Lmk what you think

ada4a added 3 commits November 2, 2025 09:28
will improve the diff for the next commit
- move `is_allowed_vec_method` (a stripped-down version of it, anyway,
  as the function doesn't make sense as is out of context) to utils, as
  it's shared between `useless_vec` and `ptr_arg`
- add another test for non-standard macro brace case
- rm unneeded `allow`s
- rm duplicated tests
- add comments to some tests
@rustbot
Copy link
Collaborator

rustbot commented Nov 2, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants