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

Nest tests/codegen/sanitizer*.rs tests in sanitizer dir #114229

Merged
merged 2 commits into from
Aug 7, 2023

Conversation

workingjubilee
Copy link
Member

The sanitizer tests are the largest and most meticulously tested set of tests in tests/codegen. That's good! They all clearly belong to a subject and thus could go in a directory, but are not, instead being placed simply in tests/codegen. That's bad! Fix this by placing them in their own directory and renaming them to be less repetitive after that move.

A few tests are brittle, and embed their filename in the test's checks. This is acceptable for the ones where it is used only two times, but one test embeds the test's mangled filename in the test over 50 times! This may have been one of the things discouraging anyone from moving it, and thus from moving the set. Fortunately, I have some knowledge of Itanium mangling (involuntarily), regex, and the FileCheck syntax. With a capturing variable, FileCheck allows us to now move this test around again without diffing it on ~50 lines, while still guaranteeing that the mangled substring is the same each time.

This also clarifies why the substring is repeated a zillion times, instead of being cryptic. They don't call it mangling because the result is pretty and easy to understand, but now it is slightly easier! Yay descriptive variables!

In the basic case, simply do the string substitution.
For one case with many instances, capture the Itanium-
mangled filename and assert its reuse instead.
@rustbot
Copy link
Collaborator

rustbot commented Jul 30, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2023
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 6, 2023

📌 Commit 4d2957c has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 7, 2023
…nest, r=Mark-Simulacrum

Nest other codegen test topics

This PR is like rust-lang#114229 in that it mostly pushes codegen tests around, shoving them into their own directories, but because all of the changes are very simple cleanups I pulled them into a separate PR. The other PR might involve actually evaluating the correctness of the test after changes, but here it is mostly a matter of taste. The only "functional" change is deleting a few tests that... hinge on a version of LLVM that we don't support (as of rust-lang#114148 anyways).

I considered a few different ways to group other topics but I feel the question of whether `tests/codegen/{vec,array,slice}` should exist is more subtle than these choices, as it might be better to group such related tests by other topics like bounds check elision, thus I avoided making it.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 7, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#98935 (Implement `Option::take_if`)
 - rust-lang#114093 (Add regression test for `echo 'mod unknown;' | rustc -`)
 - rust-lang#114229 (Nest tests/codegen/sanitizer*.rs tests in sanitizer dir)
 - rust-lang#114230 (Nest other codegen test topics)
 - rust-lang#114362 (string.rs: remove "Basic usage" text)
 - rust-lang#114365 (str.rs: remove "Basic usage" text)
 - rust-lang#114382 (Add a new `compare_bytes` intrinsic instead of calling `memcmp` directly)
 - rust-lang#114549 (Style fix and refactor on resolve diagnostics)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1371773 into rust-lang:master Aug 7, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 7, 2023
@workingjubilee workingjubilee deleted the nest-sanitizer-dir branch August 7, 2023 12:39
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Aug 8, 2023
…ark-Simulacrum

Nest other codegen test topics

This PR is like rust-lang/rust#114229 in that it mostly pushes codegen tests around, shoving them into their own directories, but because all of the changes are very simple cleanups I pulled them into a separate PR. The other PR might involve actually evaluating the correctness of the test after changes, but here it is mostly a matter of taste. The only "functional" change is deleting a few tests that... hinge on a version of LLVM that we don't support (as of rust-lang/rust#114148 anyways).

I considered a few different ways to group other topics but I feel the question of whether `tests/codegen/{vec,array,slice}` should exist is more subtle than these choices, as it might be better to group such related tests by other topics like bounds check elision, thus I avoided making it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants