r? @Manishearth

(rustbot has picked a reviewer for you, use r? to override)

r? @Centri3

Someone may care about the performance: .join() introduce an implicit clone
But personally, I don't care

☔ The latest upstream changes (presumably #11791) made this pull request unmergeable. Please resolve the merge conflicts.

Hey @lengyijun, would you mind rebasing this on master?

@ARandomDev99 This might be a good PR to review. There are some conflicts, but the implementation should be unaffected by the rebase.

If you comment something on this PR, I can also add you as an assignee

r? xFrednet

I concur with Centri3. Instead of multiple calls to PathBuf::join, it is better to suggest using PathBuf::from while dealing with only string literals. Additionally, I believe this lint should be in the pedantic category instead of complexity.

Additionally, I believe this lint should be in the pedantic category instead of complexity.

Could you explain why you feel this should be pedantic?

clippy::vec_init_then_push is warn-by-default and if this is free from FPs I think it's reasonable to have it as warn-by-default as well. We can bikeshed this part and leave it to the FCP before we merge this :)

Could you explain why you feel this should be pedantic?

@lengyijun said:

Someone may care about the performance: .join() introduce an implicit clone

This doesn't seem like a change that everyone would prefer. Having it allowed by default seems more appropriate to me. Unlike clippy::vec_init_then_push which improves performance, this lint only suggests replacing calls to push with join. This could stay in the complexity category if it replaced the calls to push with a single call to PathBuf::from.

We can bikeshed this part and leave it to the FCP before we merge this

I'm still relatively new around here. What does FCP mean? 😅

This doesn't seem like a change that everyone would prefer.

Ahh, yep you're right. Then pedantic would be the safe choice 👍

I'm still relatively new around here. What does FCP mean? 😅

No problem, FCP stands for final comment period it's a new concept for Clippy. When a new lint is ready to be merged, we create a topic on zulip to get feedback. If no concerns are raised within ~1 week we merge the PR, otherwise we discuss the issues :)

Let me know when this PR is ready to be reviewed again. I'm assuming the last commits are not ready yet due to their name :)

☔ The latest upstream changes (presumably #12999) made this pull request unmergeable. Please resolve the merge conflicts.

☔ The latest upstream changes (presumably #13080) made this pull request unmergeable. Please resolve the merge conflicts.

https://github.com/rust-lang/rust-clippy/blob/279494eced419dca11c4b8e589fc6cec9dc82f46/lintcheck/src/input.rs#L182C16-L187C19

Emmm

I assume the Emmm is referring to the comment?

You can use span_contains_comment to check if there is a comment between the creation and the push

Okay, I'd say the FCP has concluded. It sounds like the lint is okay, but we had others suggest the restriction group instead, since it might be more readable but has a performance overhead. Could you change the lint group? Then it should be ready for merge. With the addition of span_contains_comment as mentioned above :D

This version looks good to me, and the two detected cases in our CI are also very good: https://github.com/rust-lang/rust-clippy/actions/runs/10004116830?pr=11700

Thank you for the last changes. I left a tiny nit, which should be simple to fix. Then you can r=me

Roses are red,
This PR is good,
I delegate this,
To bors the bot

✌️ @lengyijun, you can now approve this pull request!

If @xFrednet told you to "r=me" after making some further change, please make that change, then do @bors r=@xFrednet

@bors r=@xFrednet

📌 Commit cb77f12 has been approved by xFrednet

It is now in the queue for this repository.

⌛ Testing commit cb77f12 with merge 8fe5c75...

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 8fe5c75 to master...