Fix some clippy warnings in libsyntax #41957

llogiq · 2017-05-12T18:14:43Z

This is mostly removing stray ampersands, needless returns and lifetimes. Basically a lot of small changes.

This is mostly removing stray ampersands, needless returns and lifetimes.

rust-highfive · 2017-05-12T18:14:53Z

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

kennytm · 2017-05-12T20:32:41Z

src/libsyntax/ext/base.rs

@@ -636,7 +636,7 @@ pub struct ExpansionData {

 /// One of these is made during expansion and incrementally updated as we go;
 /// when a macro expansion occurs, the resulting nodes have the backtrace()
-/// -> expn_info of their expansion context stored into their span.
+/// -> `expn_info` of their expansion context stored into their span.


The quote should include -> as well, probably `backtrace() -> expn_info`

Ah, good catch!

kennytm · 2017-05-12T20:37:10Z

src/libsyntax/ext/tt/macro_parser.rs

@@ -39,40 +39,40 @@
 //! Example: Start parsing `a a a a b` against [· a $( a )* a b].


Missed `…` here.

The current rendering of this doc comment is totally broken. Perhaps better just ```text … ``` the whole thing.

Good idea, will update.

kennytm · 2017-05-12T20:38:01Z

src/libsyntax/ext/tt/macro_parser.rs

-            return token::NtTT(p.parse_token_tree());
-        }
-        _ => {}
+    if let "tt" = name {


why not if name == "tt" ?

(cc Manishearth/rust-clippy#1716)

kennytm · 2017-05-12T20:40:10Z

src/libsyntax/feature_gate.rs

-                                            use marker types for now");
-                    },
-                    _ => {}
+                if let ast::ImplPolarity::Negative = polarity {


if polarity == ast::ImplPolarity::Negative { ?

kennytm · 2017-05-12T20:41:38Z

src/libsyntax/feature_gate.rs

-            };
+            if let ast::TyKind::Never = output_ty.node {
+                return
+            }
            self.visit_ty(output_ty)


if output_ty.node != ast::TyKind::Never { self.visit_ty(output_ty); }

?

kennytm · 2017-05-12T20:42:27Z

src/libsyntax/json.rs

@@ -330,7 +330,7 @@ impl DiagnosticSpanLine {
                      })
                     .collect()
             })
-            .unwrap_or(vec![])
+            .unwrap_or_else(|_| vec![])


.unwrap_or_else(Vec::new)

Same as https://github.com/rust-lang/rust/pull/41957/files#r116351937

@kennytm No, Vec::new() takes no arguments. @petrochenkov I just answered there.

kennytm · 2017-05-12T20:43:01Z

src/libsyntax/parse/classify.rs

 /// seen the semicolon, and thus don't need another.
 pub fn stmt_ends_with_semi(stmt: &ast::StmtKind) -> bool {
    match *stmt {
        ast::StmtKind::Local(_) => true,
        ast::StmtKind::Item(_) => false,
        ast::StmtKind::Expr(ref e) => expr_requires_semi_to_be_stmt(e),
-        ast::StmtKind::Semi(..) => false,
+        ast::StmtKind::Semi(..) |
        ast::StmtKind::Mac(..) => false,


Could move the ast::StmtKind::Item(_) above here.

Yeah, I usually avoid this due to the fact that it can mess with perf. But in this case it's probably fine.

kennytm · 2017-05-12T20:46:47Z

src/libsyntax/parse/mod.rs

@@ -348,20 +343,15 @@ pub fn raw_str_lit(lit: &str) -> String {

    // FIXME #8372: This could be a for-loop if it didn't borrow the iterator


This comment can be removed now? 😊 (#8372 is closed by the introduction of while let)

Ok, I'll just go and remove it.

kennytm · 2017-05-12T20:48:28Z

src/libsyntax/parse/parser.rs

-                    self.span_err(neg_span, "inherent implementation can't be negated");
-                },
-                _ => {}
+            if let ast::ImplPolarity::Negative = polarity {


if polarity == ast::ImplPolarity::Negative {

kennytm · 2017-05-12T20:50:34Z

src/libsyntax/print/pprust.rs

-                        word(&mut self.s, "!")?;
-                    },
-                    _ => {}
+                if let ast::ImplPolarity::Negative = polarity {


kennytm · 2017-05-12T21:01:48Z

src/libsyntax/ext/tt/macro_rules.rs

@@ -861,6 +858,6 @@ fn quoted_tt_to_string(tt: &quoted::TokenTree) -> String {
    match *tt {
        quoted::TokenTree::Token(_, ref tok) => ::print::pprust::token_to_string(tok),
        quoted::TokenTree::MetaVarDecl(_, name, kind) => format!("${}:{}", name, kind),
-        _ => panic!("unexpected quoted::TokenTree::{Sequence or Delimited} in follow set checker"),
+        _ => panic!("unexpected quoted::TokenTree::{{Sequence or Delimited}} in follow set checker"),


make tidy yells here, 101 chars 😑. Needs to break the string into two lines.

petrochenkov · 2017-05-13T06:19:55Z

src/libsyntax/attr.rs

+            LitKind::Bool(value) => Token::Ident(Ident::with_empty_ctxt(Symbol::intern(if value {
+                "true"
+            } else {
+                "false"


Style bikeshed: unless if cond { x } else { y } fits into one line, match is terser and more readable.

Terser, yes. More readable, I guess we'll have to agree to disagree here.

petrochenkov · 2017-05-13T06:20:30Z

src/libsyntax/diagnostics/plugin.rs

@@ -111,7 +111,7 @@ pub fn expand_register_diagnostic<'cx>(ecx: &'cx mut ExtCtxt,
    // overflow the maximum line width.
    description.map(|raw_msg| {
        let msg = raw_msg.as_str();
-        if !msg.starts_with("\n") || !msg.ends_with("\n") {
+        if !msg.starts_with('\n') || !msg.ends_with('\n') {


Why does clippy recommend this replacement?

@petrochenkov I don't see why not?

Well, in theory string search effectively works on byte arrays so "X" is already what it wants, and 'X' is a unicode character and needs to be converted into bytes first, and why would you want to deal with unicode characters at all if you just want to search for a substring.
Something like that.

@petrochenkov Ah I see. Indeed msg.starts_with('\n') generates more code. However, this should be something that rustc and/or LLVM that can be optimized out? The input is a constant ASCII character which all UTF-8-related branches should be elided.

I'm ok with backing this out, but it sounds like a perf bug to me.

petrochenkov · 2017-05-13T06:22:48Z

src/libsyntax/ext/expand.rs

@@ -454,7 +454,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
        };
        let path = &mac.node.path;

-        let ident = ident.unwrap_or(keywords::Invalid.ident());
+        let ident = ident.unwrap_or_else(|| keywords::Invalid.ident());


Why does clippy recommend this replacement?
It thinks keywords::Invalid.ident() may be "heavy"? (It's not actually, and unwrap_or looked better.)

@petrochenkov I don't think clippy is able to determine whether a function is heavy or not. It recommends unwrap_or_else whenever the inner expression is not a variable/primitive.

@petrochenkov I thought so, too, until I benchmarked a very simple expression (_.unwrap_or..(vec![])) with / without the else. Interestingly, though there's no allocation, and the result should be basically static, the closure version was a wee bit faster. Not that it matters in this place, so if you like, I can roll it back.

petrochenkov · 2017-05-13T06:27:04Z

src/libsyntax/ext/expand.rs

-    None
+    attrs.iter()
+         .position(|a| !attr::is_known(a) && !is_builtin_attr(a))
+         .map(|i| attrs.remove(i))


Style bikeshed: Closures, closures... Nobody likes good old obvious for loops anymore :(

I do think that the code better conveys the meaning now.

petrochenkov · 2017-05-13T06:28:13Z

src/libsyntax/ext/tt/macro_rules.rs

@@ -94,7 +94,7 @@ fn generic_extension<'cx>(cx: &'cx mut ExtCtxt,
                          -> Box<MacResult+'cx> {
    if cx.trace_macros() {
        let sp = sp.macro_backtrace().last().map(|trace| trace.call_site).unwrap_or(sp);
-        let mut values: &mut Vec<String> = cx.expansions.entry(sp).or_insert(vec![]);
+        let mut values: &mut Vec<String> = cx.expansions.entry(sp).or_insert_with(Vec::new);


Same as https://github.com/rust-lang/rust/pull/41957/files#r116351937

petrochenkov · 2017-05-13T06:29:55Z

src/libsyntax/feature_gate.rs

@@ -816,7 +816,7 @@ pub const BUILTIN_ATTRIBUTES: &'static [(&'static str, AttributeType, AttributeG
 ];

 // cfg(...)'s that are feature gated
-const GATED_CFGS: &'static [(&'static str, &'static str, fn(&Features) -> bool)] = &[
+const GATED_CFGS: &[(&str, &str, fn(&Features) -> bool)] = &[


Yeah, clippy warned about the complex type, but I just figured I make use of my RFC 1623 (default static lifetime in statics).

petrochenkov · 2017-05-13T06:36:46Z

src/libsyntax/parse/parser.rs

-            token::BinOp(token::And) => 1,
-            token::AndAnd => 1,
+            token::BinOp(token::And) |
+            token::AndAnd |
            _ if self.token.is_keyword(keywords::Mut) => 1,


Wait. This changes semantics.
if clause should apply only to _, not BinOp and AndAnd.

Good catch! Should be fixed now.

petrochenkov · 2017-05-13T06:37:22Z

src/libsyntax/parse/parser.rs

@@ -2571,7 +2569,7 @@ impl<'a> Parser<'a> {
                            s.print_usize(float.trunc() as usize)?;
                            s.pclose()?;
                            word(&mut s.s, ".")?;
-                            word(&mut s.s, fstr.splitn(2, ".").last().unwrap())
+                            word(&mut s.s, fstr.splitn(2, '.').last().unwrap())


Same as https://github.com/rust-lang/rust/pull/41957/files#r116351900

Hmmm...I'm ok with backing this out, but it does sound like a perf bug to me.

Wait a minute. I just benchmarked and found splitting by char be consistently faster on my machine, no matter the assembly.

petrochenkov · 2017-05-13T06:43:29Z

All my comments except for #41957 (comment) are non-blocking.

petrochenkov · 2017-05-13T20:30:35Z

Travis still fails:

tidy error: /checkout/src/libsyntax/ext/tt/macro_parser.rs:40: trailing whitespace

llogiq · 2017-05-13T20:36:28Z

Ouch, I must have re-introduced this. Fixing right now.

Mark-Simulacrum · 2017-05-14T15:16:54Z

Looks like Travis is still failing with the tidy error.

oli-obk · 2017-05-15T07:39:22Z

The build fails:

[00:02:32]    Compiling syntax v0.0.0 (file:///checkout/src/libsyntax)
[00:02:32] error: unexpected close delimiter: `}`
[00:02:32]     --> /checkout/src/libsyntax/feature_gate.rs:1454:1
[00:02:32]      |
[00:02:32] 1454 | }
[00:02:32]      | ^

llogiq · 2017-05-15T21:58:40Z

Sorry, that last push somehow got hung up on network error and completely destroyed my git repo. I took a while to get online (I don't want to clone rust's git repo on a half gig data plan), so now it'll hopefully work.

petrochenkov · 2017-05-16T05:37:59Z

Travis is still failing, due to some changes in pprust.rs, I suspect.

thread 'main' panicked at 'assertion failed: (left == right) (left: " (ref x, ref y) => (x, y, ),", right: " (ref x, ref y) => (x, y),")', /checkout/src/test/run-pass-fulldeps/qquote.rs:56

llogiq · 2017-05-16T07:04:34Z

That error seems to be related. I'll investigate.

petrochenkov · 2017-05-16T19:08:39Z

@bors r+

bors · 2017-05-16T19:08:40Z

📌 Commit 282b402 has been approved by petrochenkov

…nkov Fix some clippy warnings in libsyntax This is mostly removing stray ampersands, needless returns and lifetimes. Basically a lot of small changes.

Rollup of 5 pull requests - Successful merges: #41937, #41957, #42017, #42039, #42046 - Failed merges:

Fix some clippy warnings in libsyntax

a9c163e

This is mostly removing stray ampersands, needless returns and lifetimes.

rust-highfive assigned pnkfelix May 12, 2017

kennytm reviewed May 12, 2017

View reviewed changes

petrochenkov reviewed May 13, 2017

View reviewed changes

petrochenkov assigned petrochenkov and unassigned pnkfelix May 13, 2017

llogiq force-pushed the clippy-libsyntax branch from 2b2170d to 232e16f Compare May 13, 2017 20:29

Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 14, 2017

kennytm mentioned this pull request May 14, 2017

str::starts_with('x') (literal char) is slower than str::starts_with("x") (literal string). #41993

Closed

llogiq force-pushed the clippy-libsyntax branch from 232e16f to 4bb01cc Compare May 14, 2017 20:41

oli-obk mentioned this pull request May 15, 2017

Lint 'static lifetimes in the type of statics/consts rust-lang/rust-clippy#1762

Closed

llogiq force-pushed the clippy-libsyntax branch from 4bb01cc to aefd980 Compare May 15, 2017 10:20

adressed comments by @kennytm and @petrochenkov

958c67d

llogiq force-pushed the clippy-libsyntax branch from aefd980 to 958c67d Compare May 15, 2017 21:56

(hopefully) fix pprust error

282b402

arielb1 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 May 16, 2017

Mark-Simulacrum mentioned this pull request May 16, 2017

Rollup of 5 pull requests #42049

Merged

bors added a commit that referenced this pull request May 17, 2017

Auto merge of #42049 - Mark-Simulacrum:rollup, r=Mark-Simulacrum

d8215fc

Rollup of 5 pull requests - Successful merges: #41937, #41957, #42017, #42039, #42046 - Failed merges:

bors merged commit 282b402 into rust-lang:master May 17, 2017

llogiq deleted the clippy-libsyntax branch May 18, 2017 18:12

		@@ -39,40 +39,40 @@
		//! Example: Start parsing `a a a a b` against [· a $( a )* a b].

		@@ -348,20 +343,15 @@ pub fn raw_str_lit(lit: &str) -> String {

		// FIXME #8372: This could be a for-loop if it didn't borrow the iterator

Fix some clippy warnings in libsyntax #41957

Fix some clippy warnings in libsyntax #41957

Conversation

llogiq commented May 12, 2017

rust-highfive commented May 12, 2017

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

kennytm May 12, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennytm May 12, 2017 • edited Loading

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

petrochenkov May 13, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogiq May 13, 2017 • edited Loading

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

petrochenkov commented May 13, 2017

petrochenkov commented May 13, 2017

llogiq commented May 13, 2017

Mark-Simulacrum commented May 14, 2017

oli-obk commented May 15, 2017

llogiq commented May 15, 2017

petrochenkov commented May 16, 2017

llogiq commented May 16, 2017

petrochenkov commented May 16, 2017

bors commented May 16, 2017

kennytm May 12, 2017 •

edited

Loading

kennytm May 12, 2017 •

edited

Loading

petrochenkov May 13, 2017 •

edited

Loading

llogiq May 13, 2017 •

edited

Loading