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.

Sign up for GitHub

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

Jump to bottom

cg_llvm: Reduce visibility of all functions in the llvm module #136881

Merged
merged 2 commits into from
Feb 13, 2025
Merged

cg_llvm: Reduce visibility of all functions in the llvm module #136881

merged 2 commits into from
Feb 13, 2025

Conversation

dpaoliello
Copy link
Contributor

Next part of #135502

This reduces the visibility of all functions in the llvm module to pub(crate) and marks the enzyme_ffi modules with #![expect(dead_code)] (as previously discussed: #135502 (comment)).

r? @Zalathar

@rustbot rustbot added 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 Feb 11, 2025
@Zalathar
Copy link
Contributor

This looks good, but let's just double-check the enzyme part.

@ZuseZ4 There are a few bindings in enzyme_ffi.rs that would start giving dead-code warnings if downgraded from pub to pub(crate); are you OK with adding #![expect(dead_code)] for now to work around that?

@ZuseZ4
Copy link
Contributor

ZuseZ4 commented Feb 12, 2025 •
edited
Loading

Thanks for the heads-up. I think I'm only using them in rustc_cg_llvm anyway so pub(crate) makes sense.

Overall I find that the Expect/Allow/Prohibit(dead_code) lint is unfortunately not as smart as I'd like it to be (since it doesn't recognize things that are used, just under a currently disabled cfg). But I will find a better solution in a follow-up PR.

@Zalathar
Copy link
Contributor

Sounds good. Easy enough to change later if you prefer a different approach.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 12, 2025

📌 Commit c96d47f has been approved by Zalathar

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 Feb 12, 2025
@bors
Copy link
Contributor

bors commented Feb 12, 2025

☔ The latest upstream changes (presumably #136905) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 12, 2025
@dpaoliello
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 12, 2025
Zalathar
Zalathar reviewed Feb 13, 2025
View reviewed changes
compiler/rustc_codegen_llvm/src/llvm/ffi.rs

// Create modules.
pub fn LLVMModuleCreateWithNameInContext(ModuleID: *const c_char, C: &Context) -> &Module;
pub fn LLVMGetModuleContext(M: &Module) -> &Context;
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that LLVMGetModuleContext is being completely removed here, since it became unused in #131829.

That's fine, but I'm going to split it out into a separate patch (in this PR) so that it's easier to see in history and doesn't get hidden in the mass migration to pub(crate).

@Zalathar
Copy link
Contributor

@bors r+

If this keeps running into conflicts we might have to look into splitting it up, but for now let's just try again.

@bors
Copy link
Contributor

bors commented Feb 13, 2025

📌 Commit e7cef26 has been approved by Zalathar

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 Feb 13, 2025
@Zalathar Zalathar mentioned this pull request Feb 13, 2025
Open
jhpratt added a commit to jhpratt/rust that referenced this pull request Feb 13, 2025
@jhpratt
Rollup merge of rust-lang#136881 - dpaoliello:cleanllvm3, r=Zalathar
f7d383c 
cg_llvm: Reduce visibility of all functions in the llvm module

Next part of rust-lang#135502

This reduces the visibility of all functions in the `llvm` module to `pub(crate)` and marks the `enzyme_ffi` modules with `#![expect(dead_code)]` (as previously discussed: <rust-lang#135502 (comment)>).

r? `@Zalathar`
@jhpratt jhpratt mentioned this pull request Feb 13, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2025
@bors
Auto merge of rust-lang#136961 - jhpratt:rollup-r8rrpet, r=jhpratt
268cc49 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#134999 (Add cygwin target.)
 - rust-lang#136324 (Implement `f{16,32,64,128}::{erf,erfc}` (`#![feature(float_erf)]`))
 - rust-lang#136559 (Resolve named regions when reporting type test failures in NLL)
 - rust-lang#136660 (Use a trait to enforce field validity for union fields + `unsafe` fields + `unsafe<>` binder types)
 - rust-lang#136858 (Parallel-compiler-related cleanup)
 - rust-lang#136881 (cg_llvm: Reduce visibility of all functions in the llvm module)
 - rust-lang#136888 (Always perform discr read for never pattern in EUV)
 - rust-lang#136948 (Split out the `extern_system_varargs` feature)
 - rust-lang#136949 (Fix import in bench for wasm)

r? `@ghost`
`@rustbot` modify labels: rollup
@jhpratt jhpratt mentioned this pull request Feb 13, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2025
@bors
Auto merge of rust-lang#136965 - jhpratt:rollup-bsnqvmf, r=jhpratt
54cdc75 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#134999 (Add cygwin target.)
 - rust-lang#136559 (Resolve named regions when reporting type test failures in NLL)
 - rust-lang#136660 (Use a trait to enforce field validity for union fields + `unsafe` fields + `unsafe<>` binder types)
 - rust-lang#136858 (Parallel-compiler-related cleanup)
 - rust-lang#136881 (cg_llvm: Reduce visibility of all functions in the llvm module)
 - rust-lang#136888 (Always perform discr read for never pattern in EUV)
 - rust-lang#136948 (Split out the `extern_system_varargs` feature)
 - rust-lang#136949 (Fix import in bench for wasm)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f7d5285 into rust-lang:master Feb 13, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 13, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2025
@rust-timer
Unrolled build for rust-lang#136881
064159a 
Rollup merge of rust-lang#136881 - dpaoliello:cleanllvm3, r=Zalathar

cg_llvm: Reduce visibility of all functions in the llvm module

Next part of rust-lang#135502

This reduces the visibility of all functions in the `llvm` module to `pub(crate)` and marks the `enzyme_ffi` modules with `#![expect(dead_code)]` (as previously discussed: <rust-lang#135502 (comment)>).

r? ``@Zalathar``
@dpaoliello dpaoliello deleted the cleanllvm3 branch February 13, 2025 17:27
@scottmcm scottmcm mentioned this pull request Feb 14, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zalathar Zalathar Zalathar left review comments

Assignees

@Zalathar Zalathar

Labels
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
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.
5 participants
@dpaoliello @Zalathar @ZuseZ4 @bors @rustbot