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.

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

Add cstr! macro. #101607

Closed
wants to merge 4 commits into from
Closed

Add cstr! macro. #101607

wants to merge 4 commits into from

Conversation

reitermarkus
Copy link
Contributor

@reitermarkus reitermarkus commented Sep 9, 2022

This adds a cstr! macro for easily creating &'static Cstr.

ACP: rust-lang/libs-team#103

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 9, 2022
@rustbot
Copy link
Collaborator

rustbot commented Sep 9, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @scottmcm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2022
@rust-log-analyzer

This comment has been minimized.

@reitermarkus reitermarkus force-pushed the const-cstr branch 2 times, most recently from 067749f to 48a4823 Compare September 9, 2022 10:52
@rust-log-analyzer

This comment has been minimized.

@reitermarkus reitermarkus force-pushed the const-cstr branch 2 times, most recently from b31e960 to a4ddd84 Compare September 9, 2022 11:05
@rust-log-analyzer

This comment has been minimized.

@reitermarkus reitermarkus force-pushed the const-cstr branch 3 times, most recently from cb7e64d to 686b7e4 Compare September 9, 2022 12:42
@rust-log-analyzer

This comment has been minimized.

@reitermarkus reitermarkus force-pushed the const-cstr branch 2 times, most recently from c997c8f to 25f5b36 Compare September 9, 2022 22:32
@rust-log-analyzer

This comment has been minimized.

@reitermarkus
Copy link
Contributor Author

cc @oli-obk, since you reviewed #100291.

@klensy
Copy link
Contributor

klensy commented Sep 12, 2022

Making some functions const and adding cstr macro (isn't there already some private macro like that? Feeling like i saw it some time ago) is two independent things, should they be separated into its own PRs?

@reitermarkus
Copy link
Contributor Author

is two independent things, should they be separated into its own PRs?

Generally yes, but the macro only works if the functions are const. This will have to be rebased anyways once #100291 is merged so might be possible to split it then.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 18, 2022

Nominating for lang team discussion, to discuss if we should add something like c"abc" to the language instead of adding a macro to the library.

This would allow the tokenizer to know something is a C string, such that escape sequences like \0 can be an error, and we can allow invalid unicode like \x80.

@m-ou-se m-ou-se added I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2022
@est31
Copy link
Member

est31 commented Oct 18, 2022

Such checking can also happen in a proc macro, if the tokenizer is made tolerant of invalid characters, and the "valid unicode" check is turned into a post-parsing error. I'm one of the fans of rust-lang/rfcs#3267 :). Any capability built into the language is one that libraries can't customize.

@scottmcm
Copy link
Member

I'd still like to get inferred-type literals, since we want it for all kinds of things (like takes_nonzero(2)), so at that point we'd also be able to have takes_cstr(b"asdf") that would also do the checking, same as how takes_nonzero(0) would do checking to fail at compile-time.

@Mark-Simulacrum
Copy link
Member

IMO, it feels pretty weird for us to have a macro here rather than just have CStr::from_bytes_with_nul(b"testing\0").unwrap() as the way to do this in const contexts. If that name is too long for specific cases people can wrap that function with const fn c(s: &str) -> &CStr in the particular context.

There's probably missing const features (e.g., you can't insert the \0 to a 'static "compile time" allocation today), and unwrap() IIRC doesn't quite work, but it seems like those are generally "on the roadmap", to my knowledge. (And at least IMO writing \0 at the end isn't terrible).

@est31
Copy link
Member

est31 commented Oct 19, 2022

I've grepped the compiler for usages of the cstr! macro from the cstr crate, and could only find 10 usages. I've originally thought it would be more.

Looking over all of the ecosystem, it seems that the macro is used in "only" 224 files: https://grep.app/search?q=cstr%21&filter[lang][0]=Rust

@lopopolo
Copy link
Contributor

As another data point, I previously used the cstr crate in my own code but replaced it with qed::const_cstr_from_str! to avoid depending on a proc macro.

This crate has a similar function qed::const_cstr_from_bytes! to handle byte literals.

The bulk of my work involves oxidizing a C codebase. I would very much like easy and linenoise-free CStr construction in source code, especially in const and static contexts.

@reitermarkus
Copy link
Contributor Author

The bulk of my work involves oxidizing a C codebase.

This is also my use-case for this macro.

IMO, it feels pretty weird for us to have a macro here rather than just have CStr::from_bytes_with_nul(b"testing\0").unwrap() as the way to do this in const contexts.

CStr::from_bytes_with_nul(b"Hello, world!\0").unwrap() is not really ergonomic to use and also not usable in const contexts yet. Even if it was, when passing e.g. a *const c_char to a function, using const { CStr::from_bytes_with_nul(b"Hello, world!\0").unwrap() }.as_ptr() or defining a constant just to then pass it to a function is even less ergonomic to use.

@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/lang meeting. We are potentially interested in doing this in the future, but we don't expect to do so on a soon enough timeframe to warrant discouraging the libs team from adding this macro. Go ahead and add it if in the libs team's judgement people want it and would use it. We might add a native lang construct later, but the macro can always map to that.

@joshtriplett joshtriplett added I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. and removed I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. labels Oct 25, 2022
@reitermarkus
Copy link
Contributor Author

We are potentially interested in doing this in the future

With “this” referring to c"..." syntax?

but we don't expect to do so on a soon enough timeframe to warrant discouraging the libs team from adding this macro.

So is anything still blocking this from being merged now?

@m-ou-se
Copy link
Member

m-ou-se commented Nov 15, 2022

We are potentially interested in doing this in the future, but we don't expect to do so on a soon enough

I've opened rust-lang/rfcs#3348 to try make that happen sooner.

This PR adds a bunch of relatively hard to maintain (and hard to optimize) code to the standard library, so I'd very much prefer built in support for C string literals.

@joshtriplett
Copy link
Member

Looks like RFC 3348 is going to get accepted; closing this in favor of c"..." string literals.

@dtolnay dtolnay removed the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Sep 28, 2023
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 20, 2024
Simplify `const` `memchr`.

Extracted from rust-lang/rust#101607.

Removes the need for `const_eval_select`.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
Simplify `const` `memchr`.

Extracted from rust-lang/rust#101607.

Removes the need for `const_eval_select`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet