Split out the extern_system_varargs feature
#136948
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| // User enabled additional ABI support for varargs and function ABI matches those ones. | ||
| (true, true) => return, | ||
| ExternAbi::System { .. } if extern_system_varargs => return, |
There was a problem hiding this comment.
Could you merge these guarded arms and the unguarded arms? I think:
ExternAbi::System { .. } if extern_system_varargs => return,
ExternAbi::System { .. } => {
feature_err(&tcx.sess, sym::extern_system_varargs, span, UNSTABLE_EXPLAIN).emit();
}
Is much harder to read than:
ExternAbi::System { .. } => {
if extern_system_varargs {
return;
} else {
feature_err(&tcx.sess, sym::extern_system_varargs, span, UNSTABLE_EXPLAIN).emit();
}
Also, looking at the cotrol flow here... I think it's a bit strange here that we're issuing both a feature error and another error below. Maybe that could be consolidated/deduplicated too. Thoughts?
There was a problem hiding this comment.
There was a problem hiding this comment.
my initial thought in the way I (re)wrote it was to put all "escape paths" next to each other and then have all "error paths" next to each other.
There was a problem hiding this comment.
I tried to rewrite some things to hopefully make the thought process clearer, if this is legible to you I can iterate it a bit more to polish it out.
rustbot
added
the
T-compiler
4 tasks
workingjubilee
commented
Feb 12, 2025
Comment on lines
139
to
161
| feature_err(&tcx.sess, sym::extern_system_varargs, span, UNSTABLE_EXPLAIN).emit(); | ||
| } | ||
| } | ||
| abi => { | ||
| if abi.supports_varargs() { | ||
| // User enabled additional ABI support for varargs and function ABI matches those ones. | ||
| if extended_abi_support { | ||
| return; | ||
| } else { | ||
| // Using this ABI would be ok, if the feature for additional ABI support was enabled. | ||
| feature_err( | ||
| &tcx.sess, | ||
| sym::extended_varargs_abi_support, | ||
| span, | ||
| UNSTABLE_EXPLAIN, | ||
| ) | ||
| .emit(); | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| (false, false) => CONVENTIONS_STABLE, | ||
| (true, false) => CONVENTIONS_UNSTABLE, | ||
| }; | ||
|
|
||
| tcx.dcx().emit_err(errors::VariadicFunctionCompatibleConvention { span, conventions }); |
There was a problem hiding this comment.
Ideally we'd
- check if this is an actual error path (else bail)
- then compute the error we want to emit
- then emit one error
But also, ideally we'd not have to hardcode anything about the error we emit here: we should just filter ExternAbi::ALL_VARIANTS to "everything that actually can support varargs", then format and emit the list.
workingjubilee
force-pushed
the
split-off-extern-system-varargs
branch
2 times, most recently
from
f242874 to
154eb95
Compare
This comment has been minimized.
This comment has been minimized.
compiler-errors
approved these changes
Feb 13, 2025
|
LGTM, thanks for cleaning it up a bit @bors r+ |
bors
added
the
S-waiting-on-bors
After the stabilization PR was opened, `extern "system"` functions were added to `extended_varargs_abi_support`. This has a number of questions regarding it that were not discussed and were somewhat surprising. It deserves to be considered as its own feature, separate from `extended_varargs_abi_support`.
workingjubilee
force-pushed
the
split-off-extern-system-varargs
branch
from
fa02c12 to
4bb0c3d
Compare
|
squashed |
jhpratt
added a commit
to jhpratt/rust
that referenced
this pull request
Feb 13, 2025
…stem-varargs, r=compiler-errors Split out the `extern_system_varargs` feature After the stabilization PR was opened, `extern "system"` functions were added to `extended_varargs_abi_support`. This has a number of questions regarding it that were not discussed and were somewhat surprising. It deserves to be considered as its own feature, separate from `extended_varargs_abi_support`. Tracking issue: - rust-lang#136946
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 13, 2025
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
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 13, 2025
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
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 13, 2025
Rollup merge of rust-lang#136948 - workingjubilee:split-off-extern-system-varargs, r=compiler-errors Split out the `extern_system_varargs` feature After the stabilization PR was opened, `extern "system"` functions were added to `extended_varargs_abi_support`. This has a number of questions regarding it that were not discussed and were somewhat surprising. It deserves to be considered as its own feature, separate from `extended_varargs_abi_support`. Tracking issue: - rust-lang#136946
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After the stabilization PR was opened,
extern "system"functions were added toextended_varargs_abi_support. This has a number of questions regarding it that were not discussed and were somewhat surprising. It deserves to be considered as its own feature, separate fromextended_varargs_abi_support.Tracking issue:
extern_system_varargs#136946