Remove build-pass unnecessary comments #75673
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
@otavio |
|
Ohh okay @petrochenkov , I didn't do a deep analysis, since I don't know much about how this works. |
It's helpful to reduce the number of FIXMEs imo. |
|
@otaviopace Ah, I mean, we should revert all the changes then remove FIXMEs of the failed tests so that we could reduce FIXMEs. |
evaporei
force-pushed
the
final-check-pass
branch
from
4b16498 to
4bc6b3d
Compare
|
Okay @JohnTitor so just to see if I understood it. I should rollback all of them, even if their tests passed, but the ones which failed, I should just remove the Also, should I remove the |
Yeah, that's correct. The problem here is that we need to look at which tests should be
Yep. |
|
@JohnTitor do you know how can I analyze if I can change from If it is something too complicated to pass, or maybe you don't have context on it, we could just finish this Pull Request only with the removal of unnecessary comments. |
Well, I'd say it'd be a hard way. For example, if a test is for the const, the reviewer and you should know consts, the purpose of the test, and where will the items be resolved, evaluated, etc. |
This removal was possible, because we tried to use `check-pass` instead and the tests failed, so we can clean these comments and only leave them on tests which could be changed to `check-pass`.
evaporei
force-pushed
the
final-check-pass
branch
from
4bc6b3d to
f5d12ae
Compare
evaporei
changed the title
|
@JohnTitor so I've decided to make this Pull Request only to be to clean the tests which clearly can't use |
|
Even if they currently have differing output between check and build pass, I would prefer to avoid blanket removing the fixme. It seems likely that at least in some cases, that could be hiding bugs or unintentional differences... |
|
If that's the case I think I will close this then. |
@Mark-Simulacrum Hm you mean |
|
No, I mean that e.g. the type size printing seems like it shouldn't be build-pass and should be check-pass. I guess it can't be right now - but we shouldn't just patch over that by dropping the fixme IMO. Dropping the fixme on incremental tests could be more reasonable, but again I would prefer to check that the difference is e.g. codegen-related, and not something more subtle and earlier in the pipeline. |
|
Ah makes sense. In particular, the ICH things should be fine to be removed the FIXMEs but I might be missing something. @otaviopace So, if you want to continue, then I think we could tackle incr-comp things only here. And the detailed reviews are needed. Unfortunately, I'm not an expert on incr-comp so I couldn't help... Otherwise, we could just close this. Anyway, thanks for contributing and working on it <3 |
|
Basically I would want to see the differences/test failures before signing off on dropping any FIXME annotations here. |
|
I will close this one then, thanks for the help 😊 |
This Pull Request removes the unnecessary
build-passcomments which raised the possibility of usingcheck-pass.The way we discovered that they could be removed, was by putting
check-passinstead, and seeing the tests fail.Related issue: #62277