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

Eliminate needless lifetimes #723

Merged
merged 2 commits into from
Nov 29, 2024

Conversation

Philipp-M
Copy link
Contributor

Result from __CARGO_FIX_YOLO=1 cargo clippy --fix.
As newest nightly clippy bleats about all of this.

@waywardmonkeys
Copy link
Contributor

I've been avoiding doing this to any repo as I think it is worse and loses useful information.

I'd much rather just suppress it. There's an open issue in clippy about separating this or changing it as well.

@waywardmonkeys
Copy link
Contributor

rust-lang/rust-clippy#13514 is that issue.

@Philipp-M
Copy link
Contributor Author

I see your point, though I think it's fine for all the cases in this PR though? But I have no strong opinion here...

@DJMcNab
Copy link
Member

DJMcNab commented Nov 1, 2024

I don't really want to suppress this lint, because it does give useful results for e.g. functions.

That being said, these changes are also worse... rust-lang/rust-clippy#13286 seems to just be an unfortunate breaking change.

Copy link
Contributor

@flosse flosse left a comment

Choose a reason for hiding this comment

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

I prefer to be as close to the default as possible, but am still in agreement with other styles 😄

@Philipp-M
Copy link
Contributor Author

That being said, these changes are also worse...

You mean those in this PR?

So I think it comes down to sooner or later disable that lint or subdue to it (or add alternatives to clippy as suggested in this issue).

Should I merge this or close it?

@waywardmonkeys
Copy link
Contributor

Will leave it up to Daniel but in the other repos, my inclination is to suppress this rather than do these sorts of changes. Hoping that it gets fixed upstream.

@DJMcNab
Copy link
Member

DJMcNab commented Nov 2, 2024

If you're going to be running nightly, you should probably supress it locally until the story on the clippy side becomes clearer.

@Philipp-M
Copy link
Contributor Author

Sure that's not the issue, I can suppress it locally. So rather close this PR for now? As at least two people seem to be opposed, and I don't really care, but I like less syntax if possible (without too much loss of information).

@DJMcNab
Copy link
Member

DJMcNab commented Nov 4, 2024

I think so; we should see what decision is made on the clippy side first.

@Philipp-M Philipp-M marked this pull request as draft November 4, 2024 12:24
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I've changed my mind, I think these changes specifically are an improvement.

I think we need to be vigilant though; the example in the root of rust-lang/rust-clippy#13514 is definitely bad

@Philipp-M
Copy link
Contributor Author

So I guess in anticipation to updating the Rust version I should merge this?

@Philipp-M Philipp-M force-pushed the eliminate-needless-lifetimes branch from 7dd18f7 to f1179b2 Compare November 29, 2024 11:58
@Philipp-M Philipp-M marked this pull request as ready for review November 29, 2024 11:58
@DJMcNab
Copy link
Member

DJMcNab commented Nov 29, 2024

Yeah, that would be good.

@Philipp-M Philipp-M added this pull request to the merge queue Nov 29, 2024
Merged via the queue into linebender:main with commit e0fee74 Nov 29, 2024
17 checks passed
@Philipp-M Philipp-M deleted the eliminate-needless-lifetimes branch November 29, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants