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

A few cleanups #53246

Merged
merged 1 commit into from
Aug 14, 2018
Merged

A few cleanups #53246

merged 1 commit into from
Aug 14, 2018

Conversation

ljedrz
Copy link
Contributor

@ljedrz ljedrz commented Aug 10, 2018 •
edited
Loading

  • change skip(1).next() to nth(1)
  • collapse some if-else expressions
  • remove a few explicit returns
  • remove an unnecessary field name
  • dereference once instead of matching on multiple references
  • prefer iter().enumerate() to indexing with for
  • remove some unnecessary lifetime annotations
  • use writeln!() instead of write!()+\n
  • remove redundant parentheses
  • shorten some enum variant names
  • a few other cleanups suggested by clippy

@rust-highfive
Copy link
Collaborator

r? @KodrAus

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2018
sinkuu
sinkuu reviewed Aug 10, 2018
View reviewed changes
src/libgraphviz/lib.rs
&LabelStr(ref s) => format!("\"{}\"", s.escape_default()),
&EscStr(ref s) => format!("\"{}\"", LabelText::escape_str(&s)),
&HtmlStr(ref s) => format!("<{}>", s),
match *self {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe match_default_bindings has eliminated the need of * like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, it would compile without it too; while it's certainly easier to let the compiler do the dereferencing on its own, I wonder if doing it manually isn't faster 🤔.

@ljedrz
Copy link
Contributor Author

ljedrz commented Aug 11, 2018

r? @kennytm

@rust-highfive rust-highfive assigned kennytm and unassigned KodrAus Aug 11, 2018
kennytm
kennytm reviewed Aug 11, 2018
View reviewed changes
src/librustc_apfloat/ieee.rs Outdated
@@ -702,7 +700,7 @@ impl<S: Semantics> Float for IeeeFloat<S> {
// exponent = 1..10
// significand = 1..1
IeeeFloat {
sig: [!0 & ((1 << S::PRECISION) - 1)],
sig: [((1 << S::PRECISION) - 1)],
Copy link
Member

Choose a reason for hiding this comment

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

The outer parenthesis can be removed.

sig: [(1 << S::PRECISION) - 1],

src/librustc_target/spec/mod.rs Outdated
@@ -769,7 +769,7 @@ impl Target {
.and_then(|os| os.map(|s| s.to_string())) {
Some(val) => Ok(val),
None => {
return Err(format!("Field {} in target specification is required", name))
Err(format!("Field {} in target specification is required", name))
Copy link
Member

Choose a reason for hiding this comment

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

The match should be entirely removed in favor of ok_or_else.

obj.find(name)
    .map(|s| s.as_string())
    .and_then(|os| os.map(|s| s.to_string()))
    .ok_or_else(|| format!("Field {} in target specification is required", name))

src/libserialize/json.rs Outdated
if self.is_empty() {
false
} else {
match *self.stack.last().unwrap() {
Copy link
Member

Choose a reason for hiding this comment

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

The entire function could be replaced with

if let Some(InternalIndex(_)) = self.stack.last() {
    true
} else {
    false 
}

src/libterm/terminfo/parm.rs Outdated
Digit,
Octal,
Hex,
HEX,
Copy link
Member

Choose a reason for hiding this comment

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

... Could we make these LowerHex/UpperHex if you're going to rename it anyway.

@ljedrz
Copy link
Contributor Author

ljedrz commented Aug 11, 2018

@kennytm Thanks, all done.

@ljedrz
Copy link
Contributor Author

ljedrz commented Aug 13, 2018

@kennytm r?

@kennytm
Copy link
Member

kennytm commented Aug 14, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Aug 14, 2018

📌 Commit 535bd13 has been approved by kennytm

@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 Aug 14, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Aug 14, 2018
@kennytm
Rollup merge of rust-lang#53246 - ljedrz:cleanup_various, r=kennytm
092475f 
A few cleanups

- change `skip(1).next()` to `nth(1)`
- collapse some `if-else` expressions
- remove a few explicit `return`s
- remove an unnecessary field name
- dereference once instead of matching on multiple references
- prefer `iter().enumerate()` to indexing with `for`
- remove some unnecessary lifetime annotations
- use `writeln!()` instead of `write!()`+`\n`
- remove redundant parentheses
- shorten some enum variant names
- a few other cleanups suggested by `clippy`
@kennytm kennytm mentioned this pull request Aug 14, 2018
Closed
kennytm added a commit to kennytm/rust that referenced this pull request Aug 14, 2018
@kennytm
Rollup merge of rust-lang#53246 - ljedrz:cleanup_various, r=kennytm
e77d995 
A few cleanups

- change `skip(1).next()` to `nth(1)`
- collapse some `if-else` expressions
- remove a few explicit `return`s
- remove an unnecessary field name
- dereference once instead of matching on multiple references
- prefer `iter().enumerate()` to indexing with `for`
- remove some unnecessary lifetime annotations
- use `writeln!()` instead of `write!()`+`\n`
- remove redundant parentheses
- shorten some enum variant names
- a few other cleanups suggested by `clippy`
@kennytm kennytm mentioned this pull request Aug 14, 2018
Merged
bors added a commit that referenced this pull request Aug 14, 2018
@bors
Auto merge of #53354 - kennytm:rollup, r=kennytm
a573305 
Rollup of 11 pull requests

Successful merges:

 - #53112 (pretty print BTreeSet)
 - #53208 (Don't panic on std::env::vars() when env is null.)
 - #53226 (driver: set the syntax edition in phase 1)
 - #53229 (Make sure rlimit is only ever increased)
 - #53233 (targets: aarch64: Add bare-metal aarch64 target)
 - #53239 (rustc_codegen_llvm: Restore the closure env alloca hack for LLVM 5.)
 - #53246 (A few cleanups)
 - #53257 (Idiomatic improvements to IP method)
 - #53274 (Remove statics field from CodegenCx)
 - #53290 (Make LLVM emit assembly comments with -Z asm-comments)
 - #53317 (Mark prior failure to avoid ICE)
@bors bors merged commit 535bd13 into rust-lang:master Aug 14, 2018
@ljedrz ljedrz deleted the cleanup_various branch August 14, 2018 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sinkuu sinkuu sinkuu left review comments

@kennytm kennytm kennytm left review comments

Assignees

@kennytm kennytm

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ljedrz @rust-highfive @kennytm @bors @sinkuu @KodrAus