Skip to content

Conversation

ada4a
Copy link
Contributor

@ada4a ada4a commented Aug 20, 2025

For #7784

The first commit is for all the places with rather trivial changes:

  • replacing nested is_diagnostic_item ifs with a match on get_diagnostic_item
  • storing the result of get_diagnostic_item in a variable and reusing it

The rest of commits are for more involved changes, and for places where changing to get_diagnostic_item allowed further simplifications -- in the latter case, the follow-up commits are marked as misc:

This is roughly one evening's worth of changes, so I hope the amount won't be overwhelming^^

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Aug 20, 2025

r? @blyxyas

rustbot has assigned @blyxyas.
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 rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 20, 2025
@ada4a ada4a force-pushed the get_diagnostic_item branch from dc9b07a to 4293765 Compare August 20, 2025 19:18
@blyxyas
Copy link
Member

blyxyas commented Aug 20, 2025

Profile results: -0.13%. Performance improvement!

@ada4a
Copy link
Contributor Author

ada4a commented Aug 20, 2025

Nice to see that this actually affects performance in a noticeable way:)

@ada4a ada4a force-pushed the get_diagnostic_item branch from 4293765 to c987818 Compare August 22, 2025 13:32
@samueltardieu
Copy link
Member

Yes, calling is_diagnostic_item() repeatedly (for example in a .iter().any(…)) is really inefficient.

I've been through the PRs and they LGTM, merging. Thanks for the cleanup!

@samueltardieu samueltardieu added this pull request to the merge queue Aug 23, 2025
Merged via the queue into rust-lang:master with commit 35fb26f Aug 23, 2025
11 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 23, 2025
github-merge-queue bot pushed a commit that referenced this pull request Aug 23, 2025
continuation of #15519, see
there for the commit structure

AFAICT this finally resolves
#7784

changelog: none
@ada4a ada4a deleted the get_diagnostic_item branch August 24, 2025 06:12
@blyxyas
Copy link
Member

blyxyas commented Aug 26, 2025

15 commits for a cleanup PR with 150 changed lines is a little bit too much. Try to condense them a bit more in the future. Generally I limit my PRs to 3 commits unless there's a really good reason.

@samueltardieu
Copy link
Member

15 commits for a cleanup PR with 150 changed lines is a little bit too much. Try to condense them a bit more in the future.

Agreed. I like to see several commits when concerns are separated (and I review PRs commit per commit):

  • Each commit should lead to code that compiles and pass tests.
  • A commit which factors out some code and leads to some code changes (for example because of requirements of the new location for the code) is best kept in a separate commit in case a bug is introduced during the change and must be tracked down. However, when this is just a move and is not risky, it doesn't require a separate commit.
  • If some code could be reverted in isolation, it should be a separate commit.
  • If several major concerns are involved, they should go in several PRs that can be approved/denied on a case by case basis.

In the current case, many cleanups could have been grouped in the same commit.

@ada4a
Copy link
Contributor Author

ada4a commented Aug 26, 2025

Fair enough. My main problem with doing that is that when I have multiple changes in a single commit, I can't come up with a good commit message to describe the entirety of it. But if you deem the misc: commits to be obvious and undeserving of a description, then I could just bundle them with the actual change

github-merge-queue bot pushed a commit that referenced this pull request Sep 24, 2025
this is based #15519, but
mainly to avoid gnarly rebase conflicts later

changelog: none
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