Use `str::strip_{prefix,suffix}` when possible #74048

tesuji · 2020-07-05T02:13:29Z

No description provided.

tesuji · 2020-07-05T06:43:28Z

src/librustc_typeck/check/intrinsic.rs

+        name if {
+            suffix = name.strip_prefix("simd_shuffle");
+            suffix.is_some()
+        } =>


This works around the fact that if-let guard is not implemented (#51114).
At least it gets rid of repeating the pattern in starts_with with may lead to typos.

I continue to prefer the old code here.

Manishearth · 2020-07-05T07:12:36Z

(I usually don't do arbitrary reviews)

tesuji · 2020-07-05T13:14:04Z

Oh sorry. I thought you were interested to review this PR.

Reassign to random compiler member: r? @petrochenkov

Mark-Simulacrum · 2020-07-05T13:24:37Z

src/librustc_ast/util/comments.rs

 }

-// FIXME(#64197): Try to privatize this again.


Seems like this shouldn't be removed? It does look like it could be made private -- only use outside of here seems to be in src/librustc_expand/parse/lexer/tests.rs and those tests could be moved into this crate relatively easily I imagine.

Mark-Simulacrum · 2020-07-05T13:25:55Z

src/librustc_ast/util/comments.rs

-        || s.starts_with("//!")
-        || (s.starts_with("/**") && is_block_doc_comment(s))
-        || s.starts_with("/*!")
+    is_line_doc_comment(s) || is_block_doc_comment(s)


This seems to be a behavior change. In particular, /*! returned true before and would now return false, as is_block_doc_comment only returns true on strings >= 5 characters in length.

Mark-Simulacrum · 2020-07-05T13:28:19Z

src/librustc_ast/util/comments.rs

+    match comment
+        .strip_prefix("/**")
+        .or_else(|| comment.strip_prefix("/*!"))
+        .and_then(|s| s.strip_suffix("*/"))


This also looks like a behavior change -- I don't think we cared what the third character was before, and certainly not the last two characters.

Mark-Simulacrum · 2020-07-05T13:35:21Z

src/librustc_mir/borrow_check/diagnostics/mutability_errors.rs

-    } else {
-        None
+    match hi_src.strip_prefix("ref") {
+        Some(tail) if tail.starts_with(rustc_lexer::is_whitespace) => {


Pre-existing, but it seems odd to use rustc_lexer::is_whitespace here and char::is_whitespace above. cc @estebank

Mark-Simulacrum · 2020-07-05T13:39:29Z

src/librustc_target/abi/mod.rs

+                    remnant.is_some()
+                } =>
+                {
+                    dl.instruction_address_space = parse_address_space(remnant.unwrap(), "P")?


w/o if let guards the old code seems better, to be honest.

Mark-Simulacrum · 2020-07-05T14:11:01Z

src/librustdoc/html/render.rs

                    // Ends with "\\" (it's the case for the last added crate line)
-                    ret.push(line[..line.len() - 1].to_string());
+                    ret.push(head.to_string());


This looks like a change in behavior -- we didn't care if the character was \ before? Probably benign but I'd prefer to not change behavior at all in this patch.

Mark-Simulacrum · 2020-07-05T14:12:57Z

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -487,7 +487,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
        }

        let cx = self.cx;
-        let dox = item.attrs.collapsed_doc_value().unwrap_or_else(String::new);
+        let dox = item.collapsed_doc_value().unwrap_or_else(String::new);


Unrelated change?

Mark-Simulacrum · 2020-07-05T14:16:49Z

src/librustdoc/passes/collect_intra_doc_links.rs

-                    link.trim_start_matches("derive@")
-                } else if link.ends_with('!') {
-                    kind = Some(MacroNS);
-                    link.trim_end_matches('!')


These change behavior -- trim_start_matches will strip more than once.

Mark-Simulacrum · 2020-07-05T14:28:14Z

src/tools/clippy/clippy_dev/src/ra_setup.rs

+        //
+        // We have something like "extern crate foo;", we only care about the "foo".
+        // Also we don't trim the string cause we believe rustfmt did that for us.
+        .filter_map(|line| line.strip_prefix("extern crate ").map(|s| s.strip_suffix(';')));


This is technically a behavior change -- previously this would work if, say, the character after extern crate is a tab. Regardless I'd personally prefer that the clippy changes go into a PR against the clippy repository rather than here, we should really only be landing changes directly to the subtree if they're "required" (i.e., small patches) to avoid merge conflicts as much as possible.

Mark-Simulacrum · 2020-07-05T14:32:20Z

src/tools/compiletest/src/runtest.rs

@@ -1192,8 +1192,7 @@ impl<'test> TestCx<'test> {
        for line in reader.lines() {
            match line {
                Ok(line) => {
-                    let line =
-                        if line.starts_with("//") { line[2..].trim_start() } else { line.as_str() };
+                    let line = line.strip_prefix("//").unwrap_or(&*line).trim_start();


Behavior change: we only trimmed if stripping previously.

tesuji · 2020-07-05T18:44:51Z

Thanks for the review.
Actually I think I will split this to multiple PRs for easier reviewing.

Mark-Simulacrum · 2020-07-05T18:52:53Z

Having already reviewed most of this (modulo clippy), it would be great for those PRs to incorporate existing feedback at least. Thanks!

rust-highfive assigned Manishearth Jul 5, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 5, 2020

tesuji force-pushed the strip branch 4 times, most recently from 2657b7f to 35e6ad4 Compare July 5, 2020 06:32

tesuji commented Jul 5, 2020

View reviewed changes

tesuji force-pushed the strip branch from 35e6ad4 to 333c0e1 Compare July 5, 2020 06:58

Manishearth removed their assignment Jul 5, 2020

tesuji force-pushed the strip branch 2 times, most recently from 7f6260f to ee90da5 Compare July 5, 2020 12:59

rust-highfive assigned petrochenkov Jul 5, 2020

Use str::strip_{prefix,suffix} when possible

bb50ddf

tesuji force-pushed the strip branch from ee90da5 to bb50ddf Compare July 5, 2020 13:15

Mark-Simulacrum reviewed Jul 5, 2020

View reviewed changes

tesuji closed this Jul 5, 2020

tesuji mentioned this pull request Jul 6, 2020

Use strip_{prefix,suffix} in rustdoc #74094

Closed

tesuji deleted the strip branch June 8, 2024 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `str::strip_{prefix,suffix}` when possible #74048

Use `str::strip_{prefix,suffix}` when possible #74048

tesuji commented Jul 5, 2020 •

edited

Loading

tesuji Jul 5, 2020

Mark-Simulacrum Jul 5, 2020

Manishearth commented Jul 5, 2020

tesuji commented Jul 5, 2020

Mark-Simulacrum Jul 5, 2020

Mark-Simulacrum Jul 5, 2020

Mark-Simulacrum Jul 5, 2020

Mark-Simulacrum Jul 5, 2020

Mark-Simulacrum Jul 5, 2020

Mark-Simulacrum Jul 5, 2020

Mark-Simulacrum Jul 5, 2020

Mark-Simulacrum Jul 5, 2020

Mark-Simulacrum Jul 5, 2020

Mark-Simulacrum Jul 5, 2020

tesuji commented Jul 5, 2020

Mark-Simulacrum commented Jul 5, 2020

Use str::strip_{prefix,suffix} when possible #74048

Use str::strip_{prefix,suffix} when possible #74048

Conversation

tesuji commented Jul 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Jul 5, 2020

tesuji commented Jul 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tesuji commented Jul 5, 2020

Mark-Simulacrum commented Jul 5, 2020

Use `str::strip_{prefix,suffix}` when possible #74048

Use `str::strip_{prefix,suffix}` when possible #74048

tesuji commented Jul 5, 2020 •

edited

Loading