-
Notifications
You must be signed in to change notification settings - Fork 13.6k
tests/ui/issues/
: The Issues Strike Back [3/N]
#144400
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
base: master
Are you sure you want to change the base?
Conversation
|
This PR modifies |
☔ The latest upstream changes (presumably #144488) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
Thanks, some nits
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.
Suggestion: yeet this, this was originally testing #![feature(phase)]
that was yeeted.
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.
Question: does this still need the -2
?
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.
Suggestion: remove this test, I'm like 90% sure this already regressed in what it was testing as original reproducer is very whitespace sensitive, i.e. #14091
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.
Suggestion: also remove, whitespace was changed since a long time ago as well
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.
Suggestion: resolve/
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.
Nit: don't need the -2
then
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.
Suggestion: inference/
.
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.
This test was moved by @Oneirical and it's now tests/ui/type-inference/float-type-inference-unification-14382.rs
so I keep it as it
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.
Uh if that's the case, yeet this duplicate test
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.
After I did git pull --rebase
to remove conflicts its already unified our tests and keep their, so I just removed their comment from bottom, imo type-inference/
is fine here (https://github.com/rust-lang/rust/pull/144400/files#diff-385ae3df0488b90afc0e919de63c4398ae3f831569ce53818649706c5349afb8)
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.
Suggestion: inference/
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.
Suggestion: resolve/
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.
Suggestion: maybe lifetimes/
or bounds/
if we have such directory
Some
tests/ui/issues/
housekeeping, to trim down number of tests directly undertests/ui/issues/
. Part of #133895.r? @jieyouxu