Skip to content

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Sep 16, 2025


This is another batch of LLVMDIBuilder binding migrations, replacing some our own LLVMRust bindings with bindings to upstream LLVM-C APIs.

This PR migrates all of the bindings that were touched by #136632, plus LLVMDIBuilderCreateStructType.

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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. labels Sep 16, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 16, 2025

r? @nnethercote

rustbot has assigned @nnethercote.
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

@Zalathar Zalathar changed the title cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 2) cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 3) Sep 16, 2025

pub(crate) fn LLVMDIBuilderCreateSubroutineType<'ll>(
Builder: &DIBuilder<'ll>,
File: Option<&'ll Metadata>, // (unused)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does (unused) mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The underlying LLVM-C function ignores the File argument, discarding it with no effect.

I've rephrased this comment to // (ignored and has no effect).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(When the C API function was originally added, it needed a file argument, but subsequent changes to the underlying C++ code mean that it no longer does anything.)

}
}

fn create_pointer_type<'ll>(
Copy link
Contributor

Choose a reason for hiding this comment

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

nice encapsulation

@nnethercote
Copy link
Contributor

This stuff is unfamiliar to me but it all looks pretty straightforward. I will delegate approval just in case there's a change to made in response to my question about "(unused)".

@bors delegate=Zalathar

@bors
Copy link
Collaborator

bors commented Sep 16, 2025

✌️ @Zalathar, you can now approve this pull request!

If @nnethercote told you to "r=me" after making some further change, please make that change, then do @bors r=@nnethercote

@Zalathar
Copy link
Contributor Author

I've added some clarification to the “ignored” file parameter.

@nnethercote Did you intend to r=me this? I'm not sure what the review status of this PR is.

unsafe {
llvm::LLVMDIBuilderCreateSubroutineType(
DIB(cx),
None, // (unused)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this one to (ignored and has no effect) to match? r=me with that, thanks.

@Zalathar
Copy link
Contributor Author

@bors r=nnethercote rollup

@bors
Copy link
Collaborator

bors commented Sep 17, 2025

📌 Commit af88d14 has been approved by nnethercote

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 Sep 17, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 17, 2025
cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 3)

- Part of rust-lang#134001
- Follow-up to rust-lang#136375
- Follow-up to rust-lang#136632

---

This is another batch of LLVMDIBuilder binding migrations, replacing some our own LLVMRust bindings with bindings to upstream LLVM-C APIs.

This PR migrates all of the bindings that were touched by rust-lang#136632, plus `LLVMDIBuilderCreateStructType`.
bors added a commit that referenced this pull request Sep 17, 2025
Rollup of 14 pull requests

Successful merges:

 - #142807 (libtest: expose --fail-fast as an unstable command-line option)
 - #144871 (Stabilize `btree_entry_insert` feature)
 - #145071 (Update the minimum external LLVM to 20)
 - #145181 (remove FIXME block from `has_significant_drop`, it never encounters inference variables)
 - #145660 (initial implementation of the darwin_objc unstable feature)
 - #145838 (don't apply temporary lifetime extension rules to non-extended `super let`)
 - #146259 (Suggest removing Box::new instead of unboxing it)
 - #146410 (Iterator repeat: no infinite loop for `last` and `count`)
 - #146460 (Add tidy readme)
 - #146552 (StateTransform: Do not renumber resume local.)
 - #146564 (Remove Rvalue::Len again.)
 - #146581 (Detect attempt to use var-args in closure)
 - #146588 (tests/run-make: Update list of statically linked musl targets)
 - #146631 (cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 3))

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4e6640b into rust-lang:master Sep 17, 2025
10 checks passed
rust-timer added a commit that referenced this pull request Sep 17, 2025
Rollup merge of #146631 - Zalathar:di-builder, r=nnethercote

cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 3)

- Part of #134001
- Follow-up to #136375
- Follow-up to #136632

---

This is another batch of LLVMDIBuilder binding migrations, replacing some our own LLVMRust bindings with bindings to upstream LLVM-C APIs.

This PR migrates all of the bindings that were touched by #136632, plus `LLVMDIBuilderCreateStructType`.
@rustbot rustbot added this to the 1.92.0 milestone Sep 17, 2025
@Zalathar Zalathar deleted the di-builder branch September 17, 2025 10:45
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 18, 2025
cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 4)

- Part of rust-lang#134001
- Follow-up to rust-lang#146631

---

This is another batch of LLVMDIBuilder binding migrations, replacing some our own LLVMRust bindings with bindings to upstream LLVM-C APIs.
rust-timer added a commit that referenced this pull request Sep 18, 2025
Rollup merge of #146673 - Zalathar:di-builder, r=nnethercote

cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 4)

- Part of #134001
- Follow-up to #146631

---

This is another batch of LLVMDIBuilder binding migrations, replacing some our own LLVMRust bindings with bindings to upstream LLVM-C APIs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

4 participants