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

Convert disallowed macros to an early pass lint #132645

Closed

Conversation

jdonszelmann
Copy link
Contributor

r? @xFrednet

I forgot one yesterday, but this one also needs to be early pass. I use AttrStorage in a similar way as FormatArgs does. It's not actually very wasteful, there aren't that many attributes, and every attribute used to store its own span, which outside clippy will soon not be necessary anymore.

There's one more that needs refactoring, which @GuillaumeGomez is doing

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 5, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@GuillaumeGomez
Copy link
Member

I think fmt check will be unhappy but otherwise code looks good to me. Thanks!

@jdonszelmann
Copy link
Contributor Author

actually I don't think so, I ran tidy on it haha

@jdonszelmann
Copy link
Contributor Author

image
yup, but I'll force push over it fixing your comments

@GuillaumeGomez
Copy link
Member

Dark magic. 😆

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

This only modifies Clippy. Please open this PR towards the Clippy repo.

To do this easily you can do

git diff HEAD~1 --patch -- src/tools/clippy | sed "s|src/tools/clippy/||" > clippy.patch

and then in the Clippy repo clone

git apply /path/to/clippy.patch

@GuillaumeGomez
Copy link
Member

Oh good catch. Didn't even notice it was the wrong repository...

@flip1995
Copy link
Member

flip1995 commented Nov 5, 2024

Also the other PR should have been done to the Clippy repo.

To give an explanation for that policy: The Clippy repo runs more tests on Clippy than in the Rust repo. It also uses slightly different formatting rules. Those need to be fixed during syncing the Clippy and the Rust repo. The more changes were done between syncs to Clippy in the Rust repo, the more conflicts there are and the longer the sync takes. So only changes that must be done in the Rust repo, should be done in the Rust repo.

The most important check that is done in Clippy for changes like this: Clippy does a mini-crater run on every PR to see the effect of changes. I have the feeling that a change like this (or rather the ones in the previous PR) can have unexpected effects. Those we (meaning I) would need to catch and potentially fix during the next sync.

@jdonszelmann
Copy link
Contributor Author

jdonszelmann commented Nov 5, 2024

Oh alright. I'll refile it. I did this cause it makes immediately rebasing onto it easy as the root cause of the change is for changes in rust. Otherwise I'd have to wait for 2 weeks for the sync. But it makes sense like that :)

@jdonszelmann
Copy link
Contributor Author

closed in favor of rust-lang/rust-clippy#13657

@flip1995
Copy link
Member

flip1995 commented Nov 6, 2024

Thank you. I will do the sync tomorrow, as it is already delayed by 1 week because of other changes that blocked it, that needed to be fixed.

So you only have to wait until tomorrow evening :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants