- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.8k
          get_diagnostic_item in a bunch more places
          #15519
        
          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
Conversation
dc9b07a    to
    4293765      
    Compare
  
    | Profile results: -0.13%. Performance improvement! | 
| Nice to see that this actually affects performance in a noticeable way:) | 
avoids bounds checks
`None` is the fallback case anyway
to be able to reuse `did`
`trait_name` can only be _one_ of those at the same time
4293765    to
    c987818      
    Compare
  
    | Yes, calling  I've been through the PRs and they LGTM, merging. Thanks for the cleanup! | 
| 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. | 
| 
 Agreed. I like to see several commits when concerns are separated (and I review PRs commit per commit): 
 In the current case, many cleanups could have been grouped in the same commit. | 
| 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  | 
this is based #15519, but mainly to avoid gnarly rebase conflicts later changelog: none
For #7784
The first commit is for all the places with rather trivial changes:
is_diagnostic_itemifs with a match onget_diagnostic_itemget_diagnostic_itemin a variable and reusing itThe rest of commits are for more involved changes, and for places where changing to
get_diagnostic_itemallowed further simplifications -- in the latter case, the follow-up commits are marked asmisc:This is roughly one evening's worth of changes, so I hope the amount won't be overwhelming^^
changelog: none