Clarify documentation for `CStr` #113220

tgross35 · 2023-07-01T00:44:33Z

Better differentiate summaries for from_bytes_until_nul and from_bytes_with_nul
Add some links where they may be helpful

rustbot · 2023-07-01T00:44:41Z

r? @m-ou-se

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

tgross35 · 2023-07-01T00:45:05Z

@rustbot label +A-docs

workingjubilee

I like the idea of making these more specific but this shouldn't fudge what it means here.

workingjubilee · 2023-07-30T06:03:06Z

library/core/src/ffi/c_str.rs

+    /// Creates a C string wrapper from a byte slice that includes one or more
+    /// nul bytes.


Strictly speaking, it is valid to call this on a byte slice with zero \0 present, because the slice describes a safe bound to search in. That makes this technically incorrect. And yes, you can say "but that's the error case", but that's the problem! There's a Result!

In addition, this first line is shown first in many inline editor hints or row-by-row formatted lists and accordingly should be kept ultra-short, so I hesitate at making it so much longer. It's fine if you can't find a way to express that in fewer characters, though.

workingjubilee · 2023-07-30T06:06:43Z

library/core/src/ffi/c_str.rs

+    /// Creates a C string wrapper from a byte slice terminated by exactly one
+    /// nul byte.


Same comment on length, though if it really does need to be this long that's okay.

workingjubilee · 2023-07-30T06:07:54Z

library/core/src/ffi/c_str.rs

@@ -197,8 +197,8 @@ impl CStr {
    ///
    /// This function will wrap the provided `ptr` with a `CStr` wrapper, which
    /// allows inspection and interoperation of non-owned C strings. The total
-    /// size of the raw C string must be smaller than `isize::MAX` **bytes**
-    /// in memory due to calling the `slice::from_raw_parts` function.
+    /// size of the raw C string must be smaller than [`isize::MAX`] **bytes**


The phrase "raw C string" feels weird to me, I don't feel like this documentation adequately describes what it means. That's preexisting though so you don't have to fix it, just noticing.

workingjubilee · 2023-07-30T06:11:43Z

r? workingjubilee
@rustbot author

* Better differentiate summaries for `from_bytes_until_nul` and `from_bytes_with_nul` * Add some links where they may be helpful

tgross35 · 2023-08-02T08:00:12Z

Okiedoke I tried to improve some of the things you mentioned, should be ready for another look

@rustbot ready

workingjubilee · 2023-08-02T23:37:15Z

Thanks!

@bors r+ rollup

bors · 2023-08-02T23:37:17Z

📌 Commit c94dc72 has been approved by workingjubilee

It is now in the queue for this repository.

bors · 2023-08-03T02:40:23Z

⌛ Testing commit c94dc72 with merge d8bbef5...

bors · 2023-08-03T04:26:35Z

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing d8bbef5 to master...

rust-timer · 2023-08-03T14:31:43Z

Finished benchmarking commit (d8bbef5): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	3
Improvements ✅ (secondary)	-0.4%	[-0.6%, -0.3%]	3
All ❌✅ (primary)	-0.5%	[-0.7%, 0.2%]	4

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)	2.5%	[1.4%, 3.7%]	2
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-2.0%	[-2.2%, -1.8%]	2
Improvements ✅ (secondary)	-3.0%	[-3.0%, -3.0%]	1
All ❌✅ (primary)	0.3%	[-2.2%, 3.7%]	4

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: 647.209s -> 650.497s (0.51%)

lqd · 2023-08-03T14:48:20Z

This is a documentation change on libcore/libstd. That does affect metadata during regular compilation, but there's nothing we could do here until this is taken care of, and the single regression is fine if not noise.

@rustbot label: +perf-regression-triaged

rustbot assigned m-ou-se Jul 1, 2023

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. labels Jul 1, 2023

rustbot added the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Jul 1, 2023

workingjubilee reviewed Jul 30, 2023

View reviewed changes

rustbot assigned workingjubilee and unassigned m-ou-se Jul 30, 2023

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 30, 2023

Clarify documentation for CStr

c94dc72

* Better differentiate summaries for `from_bytes_until_nul` and `from_bytes_with_nul` * Add some links where they may be helpful

tgross35 force-pushed the cstr-bytes-docs branch from 5d65fac to c94dc72 Compare August 2, 2023 07:59

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 2, 2023

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 Aug 2, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 3, 2023

bors merged commit d8bbef5 into rust-lang:master Aug 3, 2023

rustbot added this to the 1.73.0 milestone Aug 3, 2023

tgross35 deleted the cstr-bytes-docs branch August 3, 2023 04:50

rustbot added the perf-regression Performance regression. label Aug 3, 2023

rustbot added the perf-regression-triaged The performance regression has been triaged. label Aug 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Clarify documentation for `CStr` #113220

Clarify documentation for `CStr` #113220

Uh oh!

tgross35 commented Jul 1, 2023

Uh oh!

rustbot commented Jul 1, 2023

Uh oh!

tgross35 commented Jul 1, 2023

Uh oh!

workingjubilee left a comment

Uh oh!

workingjubilee Jul 30, 2023 •

edited

Loading

Uh oh!

workingjubilee Jul 30, 2023

Uh oh!

workingjubilee Jul 30, 2023

Uh oh!

workingjubilee commented Jul 30, 2023 •

edited

Loading

Uh oh!

tgross35 commented Aug 2, 2023

Uh oh!

workingjubilee commented Aug 2, 2023

Uh oh!

bors commented Aug 2, 2023

Uh oh!

bors commented Aug 3, 2023

Uh oh!

bors commented Aug 3, 2023

Uh oh!

rust-timer commented Aug 3, 2023

Uh oh!

lqd commented Aug 3, 2023

Uh oh!

Uh oh!

		/// Creates a C string wrapper from a byte slice that includes one or more
		/// nul bytes.

		/// Creates a C string wrapper from a byte slice terminated by exactly one
		/// nul byte.

Clarify documentation for CStr #113220

Clarify documentation for CStr #113220

Uh oh!

Conversation

tgross35 commented Jul 1, 2023

Uh oh!

rustbot commented Jul 1, 2023

Uh oh!

tgross35 commented Jul 1, 2023

Uh oh!

workingjubilee left a comment

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jul 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jul 30, 2023

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jul 30, 2023

Choose a reason for hiding this comment

Uh oh!

workingjubilee commented Jul 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgross35 commented Aug 2, 2023

Uh oh!

workingjubilee commented Aug 2, 2023

Uh oh!

bors commented Aug 2, 2023

Uh oh!

bors commented Aug 3, 2023

Uh oh!

bors commented Aug 3, 2023

Uh oh!

rust-timer commented Aug 3, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

lqd commented Aug 3, 2023

Uh oh!

Uh oh!

Clarify documentation for `CStr` #113220

Clarify documentation for `CStr` #113220

workingjubilee Jul 30, 2023 •

edited

Loading

workingjubilee commented Jul 30, 2023 •

edited

Loading