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.

Sign up for GitHub

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

Jump to bottom

Implement cstr_count_bytes #114443

Merged
merged 2 commits into from
Sep 20, 2023
Merged

Implement cstr_count_bytes #114443

merged 2 commits into from
Sep 20, 2023

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Aug 3, 2023 •
edited
Loading

This has not yet been approved via ACP, but it's simple enough to get started on.

@rustbot label +T-libs-api

@rustbot
Copy link
Collaborator

rustbot commented Aug 3, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 3, 2023
@tgross35
Copy link
Contributor Author

tgross35 commented Aug 3, 2023 •
edited
Loading

@rustbot label -T-libs
r? libs-api

@rustbot rustbot assigned m-ou-se and unassigned cuviper Aug 3, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 3, 2023

Error: Parsing relabel command in comment failed: ...' label -' | error: empty label at >| ' T-libs
r'...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@rustbot rustbot removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 3, 2023
@tgross35
Copy link
Contributor Author

tgross35 commented Aug 3, 2023

I gated the constness under const_cstr_from_ptr since it has the same blocker to const stabilization.

This was referenced Aug 3, 2023
Closed
Closed
@tgross35 tgross35 force-pushed the cstr-len branch 2 times, most recently from 526250e to 21f91cf Compare August 4, 2023 01:17
asquared31415
asquared31415 reviewed Aug 4, 2023
View reviewed changes
library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
library/core/src/ffi/c_str.rs Outdated Show resolved Hide resolved
@CryZe
Copy link
Contributor

CryZe commented Aug 4, 2023 •
edited
Loading

Does it make sense for the len method to calculate the length again when &CStr already is a fat pointer knowing the length? I know the idea is to change this at some point, but I'm not sure implementing it wastefully right now makes sense instead of simply changing the implementation when the representation changes.

@tgross35
Copy link
Contributor Author

tgross35 commented Aug 4, 2023

Does it make sense for the len method to calculate the length again when &CStr already is a fat pointer knowing the length? I know the idea is to change this at some point, but I'm not sure implementing it wastefully right now makes sense instead of simply changing the implementation when the representation changes.

Yeah, that's definitely up for debate. I mentioned in the not reviewed ACP (rust-lang/libs-team#256) that I just wouldn't want to surprise users with "hey, this used to be faster but now it's O(1)!" whenever we do make that change. But I also can't imagine that affecting many people (and time complexity of other things would be changing anyway).

@tgross35 tgross35 force-pushed the cstr-len branch 3 times, most recently from dedde83 to c39b686 Compare August 10, 2023 02:54
@tgross35 tgross35 mentioned this pull request Aug 10, 2023
Closed
4 tasks
@tgross35
Copy link
Contributor Author

As recommended on the ACP, I changed this to use the constant time length but add the warning in the docs that this will change

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Aug 20, 2023

☔ The latest upstream changes (presumably #115009) made this pull request unmergeable. Please resolve the merge conflicts.

@tgross35 tgross35 changed the title Implement cstr_len Implement cstr_count_bytes Aug 29, 2023
@tgross35
Refactor the const strlen implementation to const_strlen
e8f9d1a 
Currently, `CStr::from_ptr` contains its own implementation of `strlen`
that uses `const_eval_select` to either call libc's `strlen` or use a
naive Rust implementation. Refactor that into its own function so we can
use it elsewhere in the module.
@tgross35
Implement CStr::count_bytes
fe0eb8b 
This is feature gated under `cstr_count_bytes` and provides a more
straightforward way to access the length of a `CStr`

Link: rust-lang#113219
@WaffleLapkin WaffleLapkin added S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2023
@WaffleLapkin
Copy link
Member

Marking as S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. , since the ACP was not accepted yet.

@dtolnay dtolnay assigned dtolnay and unassigned m-ou-se Sep 19, 2023
@dtolnay dtolnay added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. labels Sep 19, 2023
dtolnay
dtolnay approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thank you!

@dtolnay
Copy link
Member

dtolnay commented Sep 19, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 19, 2023

📌 Commit fe0eb8b has been approved by dtolnay

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2023
@tgross35
Copy link
Contributor Author

Thanks!

@bors
Copy link
Contributor

bors commented Sep 20, 2023

⌛ Testing commit fe0eb8b with merge 3b9e0fe...

@bors bors mentioned this pull request Sep 20, 2023
Merged
@bors
Copy link
Contributor

bors commented Sep 20, 2023

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 3b9e0fe to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 20, 2023
@bors bors merged commit 3b9e0fe into rust-lang:master Sep 20, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 20, 2023
@tgross35 tgross35 deleted the cstr-len branch September 20, 2023 02:02
@bors bors mentioned this pull request Sep 20, 2023
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3b9e0fe): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

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.

mean range count
Regressions ❌
(primary)		 3.9% [3.9%, 3.9%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -1.5% [-1.5%, -1.5%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.2% [-1.5%, 3.9%] 2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.334s -> 632.311s (0.15%)
Artifact size: 317.85 MiB -> 317.92 MiB (0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asquared31415 asquared31415 asquared31415 left review comments

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.