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

compiletest: Slightly simplify the handling of debugger directive prefixes #134849

Merged
merged 2 commits into from
Dec 29, 2024

Conversation

Zalathar
Copy link
Contributor

The cdbg- prefix is not used by any tests in tests/debuginfo, and perhaps there never were any tests that used it.

Getting rid of it also lets us get rid of the code for parsing multiple prefixes at the same time, since every debugger now has exactly one prefix.

@rustbot
Copy link
Collaborator

rustbot commented Dec 28, 2024

r? @clubby789

rustbot has assigned @clubby789.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 28, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 28, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

Copy link
Member

@lqd lqd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure who's usually in charge of this piece of code, but this looks like a good cleanup to me. If you're happy with that,

r=me

Copy link
Contributor

@clubby789 clubby789 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a nice cleanup, r=me (also) when green

Comment on lines -62 to +63
let prefixes = {
static PREFIXES: &[&str] = &["cdb", "cdbg"];
// No "native rust support" variation for CDB yet.
PREFIXES
};

// Parse debugger commands etc from test files
let dbg_cmds = DebuggerCommands::parse_from(&self.testpaths.file, self.config, prefixes)
let dbg_cmds = DebuggerCommands::parse_from(&self.testpaths.file, self.config, "cdb")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some digging, this was introduced in the very original PR that added cdb support, i.e. #60970. There was no discussion about cdbg in the PR either.

Possibilities:

In any case, none of the debuginfo tests use it.

Copy link
Contributor Author

@Zalathar Zalathar Dec 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At that time, gdb had the prefixes [gdb, gdbr, gdbg], and similarly for lldb.

I think the idea was that r would only apply when using a Rust-aware debugger, and g would only apply when using a non-Rust-aware debugger.

I imagine that the PR author copied what the other debuggers were doing, but without the r variant as there was no Rust-aware cdb (hence the mysterious comment about no "native Rust support").

Later, the whole r/g distinction was removed from gdb/lldb by #129218. But cdb was not updated, so the useless g remained.

@jieyouxu
Copy link
Member

Thanks for the cleanup!

@bors r=lqd,clubby789,jieyouxu rollup

@bors
Copy link
Contributor

bors commented Dec 28, 2024

📌 Commit 2f8824a has been approved by lqd,clubby789,jieyouxu

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 Dec 28, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 29, 2024
Rollup of 3 pull requests

Successful merges:

 - rust-lang#134849 (compiletest: Slightly simplify the handling of debugger directive prefixes)
 - rust-lang#134850 (Document virality of `feature(rustc_private)`)
 - rust-lang#134852 (Added a codegen test for optimization with const arrays)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a8a1011 into rust-lang:master Dec 29, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 29, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 29, 2024
Rollup merge of rust-lang#134849 - Zalathar:debuginfo-prefixes, r=lqd,clubby789,jieyouxu

compiletest: Slightly simplify the handling of debugger directive prefixes

The `cdbg-` prefix is not used by any tests in `tests/debuginfo`, and perhaps there never were any tests that used it.

Getting rid of it also lets us get rid of the code for parsing multiple prefixes at the same time, since every debugger now has exactly one prefix.
@Zalathar Zalathar deleted the debuginfo-prefixes branch December 29, 2024 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants