-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Expand list of trait implementers in E0277 when calling rustc with --verbose #126055
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't help but point out that this original logic was/is a little goofy, in that it says: "if the length is 9 or less, keep it. If its greater than 9, then cut it to length 8."
Why wouldn't you cut it to length 9? (Or use "8 or less" as the relevant threshold.)
(My current guess is that it was an off-by-one error in some past person's end in terms of whether
candidates[..end]
would include or exclude theend
itself; otherwise I cannot explain why someone would have written the code like this.)this has the effect, I think, that will won't actually cut to length 8 until you have 10 elements; a 9-element list is printed in full.
This is not really relevant to this PR though, apart from the fact that reviewing this code tempted me to go in and rewrite it all. But I'm not going to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they might've wanted to avoid a message like
and 1 others
which would've needed some rewording such asand 1 other
(lacking ans
ending), so then avoid the more complicated logic with that extras
.Thus, show 8 + the 2 others msg if above 9, or show all 9 or less with no msg. Makes some sense :D and is kinda clever.
oh and btw, I had the exact same thought as you initially, but I wasn't satisfied with the off-by one so I had to search for another reason but still without actually looking at the relevant PR or blame because I was kinda lazy, admittedly (I still haven't looked, I wonder tho, if they did mention it), ok wth, I'm gonna look now...will edit later
EDIT: tracking this down isn't easy lol, but here's the result so far:
It might appear that the source of this is this commit: 3aac307
but it seems it was only improving on something that was originally from this commit: ca54fc76ae30 and thus this PR #39804
So, given the above then, we can conclude that it was even more than just worrying about the extra
s
, but rather, showing that line, replaced one possible type being listed, so it was useless, thus showing 2 or more was needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't to avoid the "1 others" message, but rather if we're already "wasting" a line to say there's another one, then just print it.