refactor: Remove LLVMRustInsertPrivateGlobal and define_private_global
#147488
Conversation
rustbot
added
A-LLVM
|
r? @SparrowLii rustbot has assigned @SparrowLii. Use |
…obal` Since it can easily be implemented using the existing LLVM C API in terms of `LLVMAddGlobal` and `LLVMSetLinkage` and `define_private_global` was only used in one place.
AMS21
force-pushed
the
remove_llvm_rust_insert_private_global
branch
from
101b8d5 to
036ab3a
Compare
AMS21
changed the title
LLVMRustInsertPrivateGlobalLLVMRustInsertPrivateGlobal and define_private_global
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
Rollup of 12 pull requests Successful merges: - #146568 (Port the implemention of SIMD intrinsics from Miri to const-eval) - #147373 (give a better example why `std` modules named like primitives are needed) - #147419 (bootstrap: add `Builder::rustc_cmd` that includes the lib path) - #147420 (Add diagnostic items for `pub mod consts` of FP types) - #147457 (specialize slice::fill to use memset when possible) - #147467 (Fix double warnings on `#[no_mangle]`) - #147470 (Clarify how to remediate the panic_immediate_abort error) - #147480 (Do not invalidate CFG caches in CtfeLimit.) - #147481 (format: some small cleanup) - #147488 (refactor: Remove `LLVMRustInsertPrivateGlobal` and `define_private_global`) - #147489 (Prefer to use repeat_n over repeat().take()) - #147506 (compiletest: Isolate public APIs and minimize public surface area) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #147488 - AMS21:remove_llvm_rust_insert_private_global, r=nikic refactor: Remove `LLVMRustInsertPrivateGlobal` and `define_private_global` Since it can easily be implemented using the existing LLVM C API in terms of `LLVMAddGlobal` and `LLVMSetLinkage` and `define_private_global` was only used in one place. Work towards #46437
Rollup of 12 pull requests Successful merges: - rust-lang/rust#146568 (Port the implemention of SIMD intrinsics from Miri to const-eval) - rust-lang/rust#147373 (give a better example why `std` modules named like primitives are needed) - rust-lang/rust#147419 (bootstrap: add `Builder::rustc_cmd` that includes the lib path) - rust-lang/rust#147420 (Add diagnostic items for `pub mod consts` of FP types) - rust-lang/rust#147457 (specialize slice::fill to use memset when possible) - rust-lang/rust#147467 (Fix double warnings on `#[no_mangle]`) - rust-lang/rust#147470 (Clarify how to remediate the panic_immediate_abort error) - rust-lang/rust#147480 (Do not invalidate CFG caches in CtfeLimit.) - rust-lang/rust#147481 (format: some small cleanup) - rust-lang/rust#147488 (refactor: Remove `LLVMRustInsertPrivateGlobal` and `define_private_global`) - rust-lang/rust#147489 (Prefer to use repeat_n over repeat().take()) - rust-lang/rust#147506 (compiletest: Isolate public APIs and minimize public surface area) r? `@ghost` `@rustbot` modify labels: rollup
| gv | ||
| } | ||
| _ => self.define_private_global(self.val_ty(cv)), | ||
| _ => self.define_global("", self.val_ty(cv)).unwrap_or_else(|| { |
There was a problem hiding this comment.
What on Earth does it mean to give a global the empty string as the name?!? This is not documented anywhere that I could find. (I checked define_global and a few of the functions it calls.)
There was a problem hiding this comment.
The change done after @nikic's review comment doesn't seem to match what was suggested.
There was a problem hiding this comment.
An empty string name is how you create an anonymous symbol.
This is the same thing the old code did, it was just hidden behind a const Twine &Name = "" default value.
There was a problem hiding this comment.
Does "anonymous" mean "guaranteed fresh"?
And get_defined_value("") always returns None?
The bug! below this line makes it sound like "anonymous global symbol is already defined" is an actual error condition, but if it is truly fresh then that adds to my confusion.
There was a problem hiding this comment.
Yes. This kind of global is considered "unnamed" and will be rendered as @42 or similar when printed. The bug! here can't fail.
Do I understand correctly that the suggestion here is to replace the call to define_global() with declare_global(), which doesn't check existence of the global first, which also means we don't need this unwrap?
There was a problem hiding this comment.
I hadn't even though of that, but that sounds like a good idea -- together with documentation for declare_global that says what happens when the name is empty.
7 participants
Since it can easily be implemented using the existing LLVM C API in
terms of
LLVMAddGlobalandLLVMSetLinkageanddefine_private_globalwas only used in one place.
Work towards #46437