Skip to content

Conversation

Darksonn
Copy link
Contributor

@Darksonn Darksonn commented Jun 16, 2025

As a follow-up to the Location::file_with_nul ACP, I'm filing this PR to ask a question for T-libs-api:

Should the NUL-terminated file location returned by core::panic::Location use the &CStr type or the raw pointer type?

This question is listed as an unresolved question in the ACP and tracking issue.

The main advantage of a raw pointer is that creating a &CStr requires knowing the length. This means that if we reintroduce the optimization from #117431 of not storing the length anywhere, callers of file_with_nul will make an unnecessary call to strlen when they just need to pass the pointer into C code. The main disadvantage is that using the raw pointer is unsafe.

Tracking issue: #141727

@Darksonn Darksonn added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. A-panic Area: Panicking machinery A-rust-for-linux Relevant for the Rust-for-Linux project labels Jun 16, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 16, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 16, 2025

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@a1phyr
Copy link
Contributor

a1phyr commented Jun 17, 2025

callers of file_with_nul will make an unnecessary call to strlen when they just need to pass the pointer into C code.

I don't think that this is really a disadvantage, as the compiler is perfectly able to remove an unnecessary call to strlen (eg see https://godbolt.org/z/3ezaz1esa).

@m-ou-se
Copy link
Member

m-ou-se commented Jun 17, 2025

There is still a plan to make &CStr a thin pointer that does not know the string length. It's just blocked on figuring out how to make !Sized types more flexible.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 17, 2025

We discussed this in the libs-api meeting. There wasn't much enthusiasm for this change.

Part of the reason is that we might still make &CStr a thin pointer. Even if we don't, using a raw pointer here is a very inconvenient type. Reducing the size of Location by not storing the length doesn't seem to have much advantages, and does mean having to use strlen for the regular file() -> &str method. And even if we do end up going that route (while keeping &CStr wide), the strlen call can be easily optimized away as pointed out above.

Because of these reasons, I'm closing this PR.

@m-ou-se m-ou-se closed this Jun 17, 2025
@Darksonn
Copy link
Contributor Author

Thanks! Marking the feature as implementation complete in the tracking issue.

@Darksonn Darksonn removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jun 19, 2025
@Darksonn Darksonn deleted the file-ptr-len branch June 19, 2025 07:55
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 28, 2025
… r=Mark-Simulacrum

Do not include NUL-terminator in computed length

This PR contains just the first commit of rust-lang#142579 which changes it so that the string length stored in the `Location` is the length of the `&str` rather than the length of the `&CStr`. Since most users will want the `&str` length, it seems better to optimize for that use-case.

There should be no visible changes in the behavior or API.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 28, 2025
… r=Mark-Simulacrum

Do not include NUL-terminator in computed length

This PR contains just the first commit of rust-lang#142579 which changes it so that the string length stored in the `Location` is the length of the `&str` rather than the length of the `&CStr`. Since most users will want the `&str` length, it seems better to optimize for that use-case.

There should be no visible changes in the behavior or API.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 28, 2025
… r=Mark-Simulacrum

Do not include NUL-terminator in computed length

This PR contains just the first commit of rust-lang#142579 which changes it so that the string length stored in the `Location` is the length of the `&str` rather than the length of the `&CStr`. Since most users will want the `&str` length, it seems better to optimize for that use-case.

There should be no visible changes in the behavior or API.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 28, 2025
… r=Mark-Simulacrum

Do not include NUL-terminator in computed length

This PR contains just the first commit of rust-lang#142579 which changes it so that the string length stored in the `Location` is the length of the `&str` rather than the length of the `&CStr`. Since most users will want the `&str` length, it seems better to optimize for that use-case.

There should be no visible changes in the behavior or API.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 28, 2025
… r=Mark-Simulacrum

Do not include NUL-terminator in computed length

This PR contains just the first commit of rust-lang#142579 which changes it so that the string length stored in the `Location` is the length of the `&str` rather than the length of the `&CStr`. Since most users will want the `&str` length, it seems better to optimize for that use-case.

There should be no visible changes in the behavior or API.
rust-timer added a commit that referenced this pull request Jun 29, 2025
Rollup merge of #142708 - Darksonn:location-len-without-nul, r=Mark-Simulacrum

Do not include NUL-terminator in computed length

This PR contains just the first commit of #142579 which changes it so that the string length stored in the `Location` is the length of the `&str` rather than the length of the `&CStr`. Since most users will want the `&str` length, it seems better to optimize for that use-case.

There should be no visible changes in the behavior or API.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jun 29, 2025
…imulacrum

Do not include NUL-terminator in computed length

This PR contains just the first commit of rust-lang/rust#142579 which changes it so that the string length stored in the `Location` is the length of the `&str` rather than the length of the `&CStr`. Since most users will want the `&str` length, it seems better to optimize for that use-case.

There should be no visible changes in the behavior or API.
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Jul 4, 2025
… r=Mark-Simulacrum

Do not include NUL-terminator in computed length

This PR contains just the first commit of rust-lang#142579 which changes it so that the string length stored in the `Location` is the length of the `&str` rather than the length of the `&CStr`. Since most users will want the `&str` length, it seems better to optimize for that use-case.

There should be no visible changes in the behavior or API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-panic Area: Panicking machinery A-rust-for-linux Relevant for the Rust-for-Linux project S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

5 participants