-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Add Location::file_with_nul #135054
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 Location::file_with_nul #135054
Conversation
Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred to the CTFE machinery cc @rust-lang/wg-const-eval |
Renamed to |
d28ac01
to
73810aa
Compare
How does this compare with #131828? |
@bjorn3 This is essentially the same feature, although this implementation doesn't require an increase to the size of the |
The other PR does not change the size of the |
library/core/src/panic/location.rs
Outdated
let str_len = self.file_bytes_with_nul.len() - 1; | ||
// SAFETY: `file_bytes_with_nul` without the trailing nul byte is guaranteed to be | ||
// valid UTF8. | ||
unsafe { crate::str::from_raw_parts(self.file_bytes_with_nul.as_ptr(), str_len) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
str
is allowed to have null bytes within. So you could keep the type at &'a str
and use &self.file[..self.file.len() -1]
here without unsafe code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could! I think that just moves the unsafety into a call to from_bytes_with_nul_unchecked
for the CStr, though, so I think it's roughly the same either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That unsafety exists either way unless you just directly encode it as CStr, tho we're still holding out for that becoming a thin pointer, at which point it would be expensive to get the str out of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's simpler to just store a raw pointer and length without the nul-terminator included. That way, Location
hold the integer we want most of the time without triggering provenance concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That way,
Location
hold the integer we want most of the time without triggering provenance concerns.
What integer do you mean?
I wasn't aware of any provenance concerns, was that discussed in #131828?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The provenance discussion is in a hidden subthread above: #135054 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that storing a raw pointer / length instead of a slice would let us store the length without the nul-terminator included, which makes more sense to me.
Thanks for the correction! I must've misread it when I saw it. In either case, I don't have a strong preference and am happy to remove this one if it makes life easier. I didn't know about the first PR when I wrote this one. |
I closed my original PR in favor of this PR, and opened a new PR #135240 that shows how the alternative approach of adding a compiler flag would look. |
Let's do a perf run to get a concrete binary size overhead (which will of course be so tiny that it's negligible, but having proof of that seems to be necessary in this discussion!). |
This comment has been minimized.
This comment has been minimized.
Add Location::file_with_nul This is useful for C/C++ APIs which expect the const char* returned from __FILE__ or std::source_location::file_name.
☀️ Try build successful - checks-actions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me after adding tracking issue and removing the cfg attributes ❤️
This comment has been minimized.
This comment has been minimized.
This is useful for C/C++ APIs which expect the const char* returned from __FILE__ or std::source_location::file_name.
Add Location::file_with_nul This is useful for C/C++ APIs which expect the const char* returned from __FILE__ or std::source_location::file_name. ACP: rust-lang/libs-team#466 Tracking issue: #141727
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 425e142 (parent) -> c360e21 (this PR) Test differencesShow 12 test diffsStage 1
Stage 2
Additionally, 10 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard c360e219f5a56631baa46065d28e9852ca7d4ce3 --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (c360e21): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -1.2%, secondary -3.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 3.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary -0.1%, secondary -0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 749.811s -> 750.544s (0.10%) |
This slightly improved the binary size. Likely because the string merging linker optimization now kicks in. |
This is useful for C/C++ APIs which expect the const char* returned from FILE or std::source_location::file_name.
ACP: rust-lang/libs-team#466
Tracking issue: #141727