Skip to content
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

Split tests/ieee.rs into smaller files #93849

Closed

Conversation

yerke
Copy link
Contributor

@yerke yerke commented Feb 10, 2022

Part of #60302

@rust-highfive
Copy link
Contributor

Changes rustc_apfloat. rustc_apfloat is currently in limbo and you almost certainly don't want to change it (see #55993).

cc @eddyb

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 10, 2022
@rust-highfive
Copy link
Contributor

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 10, 2022
@nagisa
Copy link
Member

nagisa commented Feb 11, 2022

This seems like a reasonable thing to do, but I'm not sure if splitting into numbered files is the right approach. At the very least I would like to see more meaningful file/module names and grouping.

Another concern I have is that a change such as this will cause, if I recall correctly, the build system to link multiple test binaries, thus increasing build times. I feel like these tests should be moved from integration tests back into the in-library #[test] modules.

@eddyb
Copy link
Member

eddyb commented Feb 12, 2022

r? @eddyb

Splitting files up just for the sake of it doesn't make sense to me. There's nothing inherently bad about long files, only about lack of organization, and just splitting the files up doesn't introduce more organization. The tidy check is merely a poor approximation of a vague metric, not some kind of golden rule.

As anecdotal evidence, I've never really run into issues with very long files, but have had time wasted by unexpected "this part of the code is in some other random place I didn't expect to exist".

Also, organization mostly matters for areas of active development. rustc_apfloat is quite the opposite of that.

In fact, you can consider rustc_apfloat to be in minimal-maintenance mode (see also #55993). As such, PRs that are not required to keep the build working, should probably not be accepted. The only exception I can think of is if you can find some additional organization in LLVM that could be replicated here (but I'm also wary about backports given the whole licensing stuff).

@rust-highfive rust-highfive assigned eddyb and unassigned nagisa Feb 12, 2022
@workingjubilee
Copy link
Member

ieee2.rs is an interesting choice of name, at the very least, when the ieee part is short for "IEEE 754-2008", I believe, which is indeed the 2nd revision of the standard, but both of these would be testing conformance to that...

@yerke
Copy link
Contributor Author

yerke commented Feb 12, 2022

@eddyb The motivation to get rid of // ignore-tidy-filelength was described in #60302, which I linked in the description.
Do you want me to close this PR?

@yerke yerke closed this Feb 12, 2022
@yerke yerke deleted the yerke/split-ieee-tests-into-smaller-files branch February 12, 2022 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants