rust (original) (raw)

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.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

#138446

Rust-for-linux request: Rust-for-Linux/linux#355

-Zunsigned-char=unsigned|signed|default flag is a target modifier. The flag generates configuration option unsigned_char to be used in primitives.rs c_char-to-i8/u8 selection.

cfg!(unsigned_char = "unsigned")
cfg!(unsigned_char = "signed")
cfg!(unsigned_char = "default")

Also, it needs to correct primitives.rs in libc crate in the same way to have consistent libc::c_char.

r? @fee1-dead

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

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.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

Mar 9, 2025

@@ -470,5 +473,7 @@ impl CheckCfg {

ins!(sym::unix, no_values);
ins!(sym::windows, no_values);

ins!(sym::unsigned_char, empty_values).extend(UnsignedCharVar::all());

Choose a reason for hiding this comment

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

the cfg needs to be unstable for now

Choose a reason for hiding this comment

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

You mean, to hide it behind unstable feature, right?

Choose a reason for hiding this comment

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

yes, using cfg(unsigned_char) should require using an unstable feature. there are other cfgs like this (I do not remember which) where you can look at how to do it

Choose a reason for hiding this comment

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

c_char_type unstable feature added and it is now a feature gated cfg.

This comment has been minimized.

I don't like the name unsigned-char because the flag configures c_char and not c_uchar. What is the reason behind the name?

I don't like the name unsigned-char because the flag configures c_char and not c_uchar. What is the reason behind the name?

For me, unsigned-char looks better and is better greppable and corresponds to -funsigned-char in gcc/clang, but yes, has some ambigity.

Do you think, something like -Zc-char=unsigned|signed would be better?

Yeah, something like c-char or c-char-type would be better IMO

azhogin changed the title~~-Zunsigned-char=unsigned|signed|default flag for c_char->u8/i8 selection override~~ -Zc-char-type=unsigned|signed|default flag for c_char->u8/i8 selection override

Mar 13, 2025

@@ -25,6 +25,8 @@ optimize_for_size = []
debug_refcell = []
# Make `TypeId` store a reference to the name of the type, so that it can print that name.
debug_typeid = []
#Allow -Zc-char-type='unsigned\|signed

Choose a reason for hiding this comment

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

Why do we need a compiler flag? As far as I can see, this (and the other change in library/core/src/ffi/primitives.rs) are the only things actually needed. Given libcore is precompiled, I don't understand what effect -Zc-char-type could actually have.

Choose a reason for hiding this comment

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

Good point, maybe a feature flag for core would be sufficient for RFL's needs?

Choose a reason for hiding this comment

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

This option also need to be consistent with libc c_char. With flag, this consistency will be provided by target modifiers system.
May feature flag mechanics secure consistency of core c_char with libc c_char?
Is there an example of existing feature flag with similar logic?

Choose a reason for hiding this comment

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

The Rust compiler doesn't have special knowledge of the libc crate so it's not really in a position to enforce that. Maybe libc should just re-export the type from core::ffi?

cc @tgross35 since you've been working on the libc crate lately 🙂

Choose a reason for hiding this comment

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

I'd like to reexport core::ffi for now and then just drop the libc types entirely for 1.0, but we can't make these changes yet due to MSRV and compat concerns. That should eventually be able to happen though so I wouldn't make any decisions here based on libc, IOW a library flag seems reasonable 👍 (assuming the compiler doesn't need knowledge of this for whatever reason).

Some more details: rust-lang/libc#4257