Skip to content

Conversation

AMS21
Copy link
Contributor

@AMS21 AMS21 commented Oct 10, 2025

Since LLVMRustContextCreate can easily be replaced with a call to LLVMContextCreate and LLVMContextSetDiscardValueNames.

Work towards #46437

@rustbot
Copy link
Collaborator

rustbot commented Oct 10, 2025

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@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 Oct 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 10, 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

Copy link
Contributor

@nnethercote nnethercote 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 no expert on this, but is this actually an improvement? LLVMRustContextCreate isn't mentioned in #46437. And #46437 is now almost eight years old, and very little progress has apparently been made on it, which maybe suggests that it's not so important?

if a PR was filed that converted a whole bunch of the functions mentioned in #46437, that might be more compelling. But changing a single function like this... is it really worthwhile?

View changes since this review


unsafe extern "C" {
// Create and destroy contexts.
pub(crate) fn LLVMContextCreate() -> &'static mut Context;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remark: Seeing &'static mut is always terrifying, but since the existing LLVMRust binding already does the same thing, I guess this isn't making things any worse.

I've been planning some changes of my own to replace these with raw pointers, but I can just as easily do that after merging this PR, so this is fine.

@Zalathar
Copy link
Contributor

I'm no expert on this, but is this actually an improvement? LLVMRustContextCreate isn't mentioned in #46437. And #46437 is now almost eight years old, and very little progress has apparently been made on it, which maybe suggests that it's not so important?

if a PR was filed that converted a whole bunch of the functions mentioned in #46437, that might be more compelling. But changing a single function like this... is it really worthwhile?

IMO there is a modest benefit in making this change, since removing LLVMRust bindings makes it easier to audit the ones that remain.

And in fact I was already planning a similar change of my own, so I'm willing to vouch for this being worthwhile.

@Zalathar
Copy link
Contributor

r? Zalathar

@rustbot rustbot assigned Zalathar and unassigned nnethercote Oct 10, 2025
Since `LLVMRustContextCreate` can easily be replaced with a call to
`LLVMContextCreate` and `LLVMContextSetDiscardValueNames`.
@AMS21 AMS21 force-pushed the remove_llvm_rust_context_create branch from 5f8c98b to 0abecda Compare October 10, 2025 13:46
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-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.

4 participants