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.

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

Cargo clippy changes #308

Open
wants to merge 4 commits into
base: master
from
Open

Conversation

@elichai
Copy link
Collaborator

elichai commented Aug 5, 2019

Hi,
This PR includes a bunch of stuff,
the first thing is to see how people feel about clippy suggestions, unlike rustfmt clippy's suggestions don't change that often and contain a lot of useful suggestions.

Commit e5ad634 should increase performance by batching allocations.
(https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization, https://rust-lang.github.io/rust-clippy/master/index.html#map_entry)

Commit c9ba8ec might reduce memory usage by removing needless references and pass by value small Copy objects.

Commit ae26c7d is one that I think is important, PartialEq and Hash must agree (meaning that a==a means that Hash(a)==Hash(a)) so it is really discouraged to manually implement one but derive the other,
and since we didn't do anything special there I preferred to derive them both.
(https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq)

Commit bf82f63 contains idiomatic suggestions, some might improve performance (like using char instead of strings in split()), and some might improve reliability (using b'a' instead of 'a' as u8)
(https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern https://rust-lang.github.io/rust-clippy/master/index.html#char_lit_as_u8 )

The last commit 6e234f7 I'm not sure how I feel about it I decided to add it, though it can be remove if wanted. (dropped)

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 5, 2019

Codecov Report

Merging #308 into master will increase coverage by 0.82%.
The diff coverage is 84.05%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #308      +/-   ##
=========================================
+ Coverage   85.98%   86.8%   +0.82%     
=========================================
  Files          40      40              
  Lines        8083    9170    +1087     
=========================================
+ Hits         6950    7960    +1010     
- Misses       1133    1210      +77
Impacted Files Coverage Δ
src/util/base58.rs 82.16% <ø> (ø) ⬆️
src/util/psbt/macros.rs 100% <ø> (ø) ⬆️
src/util/psbt/map/global.rs 65.87% <0%> (+4.5%) ⬆️
src/util/psbt/map/output.rs 68.33% <0%> (+5.17%) ⬆️
src/util/psbt/mod.rs 89.76% <0%> (ø) ⬆️
src/network/address.rs 98.88% <100%> (ø) ⬆️
src/util/hash.rs 93.75% <100%> (ø) ⬆️
src/util/address.rs 87% <100%> (-0.88%) ⬇️
src/network/message.rs 96.7% <100%> (+1.41%) ⬆️
src/util/misc.rs 95.31% <100%> (ø) ⬆️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b946b0...1a83e26. Read the comment docs.

src/blockdata/script.rs Outdated Show resolved Hide resolved
src/util/amount.rs Outdated Show resolved Hide resolved
src/util/bip32.rs Outdated Show resolved Hide resolved
Copy link
Contributor

dpc left a comment

I have some comments, but except for that one lint, LGTM

Ok(if negative {
SignedAmount(-(satoshi as i64))
} else {
SignedAmount(satoshi as i64)

This comment has been minimized.

Copy link
@apoelstra

apoelstra Aug 5, 2019

Member

I don't feel strongly about this, but why is 5 lines better than 4?

This comment has been minimized.

Copy link
@elichai

elichai Aug 5, 2019

Author Collaborator

supposedly matching over a bool is less readable. I think that sometimes match can be more readable, don't have strong feelings about this too https://rust-lang.github.io/rust-clippy/master/index.html#match_bool

inp.parse().map_err(|_| Error::InvalidChildNumberFormat)?
)?,
Ok(if inp.chars().last().map_or(false, |l| l == '\'' || l == 'h') {
ChildNumber::from_hardened_idx(inp[0..inp.len() - 1].parse().map_err(|_| Error::InvalidChildNumberFormat)?)?

This comment has been minimized.

Copy link
@apoelstra

apoelstra Aug 5, 2019

Member

What is our line length limit?

This also feels harder to read than the original.

This comment has been minimized.

Copy link
@elichai

elichai Aug 5, 2019

Author Collaborator

I felt like both this and the original are unreadable, but I didn't want to actually format code in this PR, I think that after formatting the if will be more readable
(in formatting I mean new lines and spacing)

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 13, 2019

Collaborator

Default Rust line length limit is 100 chars. That's also what I used in #290.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Aug 13, 2019

Member

How would people feel about dropping it to 80?

This comment has been minimized.

Copy link
@elichai

elichai Aug 13, 2019

Author Collaborator

Personally I don't like short lines. especially in function declarations/call. I hated this so much in PEB8 when I wrote python.
I think we should use common sense.
when you do stuff like the builder pattern than adding a space for every function is good, when you declare a function than passing the max length by 5-10 chars is fine in my opinion, we all have FHD/QHD+ displays here.

Anyway this is a rustfmt thing, not a clippy thing.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Aug 13, 2019

Member

we all have FHD/QHD+ displays here

But some of us like to view multiple files at once ;)

Anyway sure, I'm fine with 100 lines. I'm just annoyed when rustfmt does stuff like collapsing a "common sense" multi-line builder pattern into a 100-line mess.

This comment has been minimized.

Copy link
@elichai

elichai Aug 13, 2019

Author Collaborator

yeah, even in my personal projects where I have a rustfmt.toml file I still find myself manually overriding stuff that it do

This comment has been minimized.

Copy link
@dpc

dpc Aug 13, 2019

Contributor

@apoelstra It's possible to tweak this, I believe. use_small_heuristics=off https://rust-lang.github.io/rustfmt/ ?

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 14, 2019

Collaborator

I use use_small_heuristic = "Off" in all my projects. I don't like rustfmt to "heuristically" decide whether a line "feels" short/long.

I'm personally not in favor of shortening it to 80, though. Sadly rustfmt doesn't support a separate line length for comments, where I do prefer 80.

80 is just really too short for any method declaration involving Result..

@apoelstra

This comment has been minimized.

Copy link
Member

apoelstra commented Aug 5, 2019

NAK the "remove redundant field names". This is one of the worst misfeatures in Rust's syntax.

@dpc

This comment has been minimized.

Copy link
Contributor

dpc commented Aug 5, 2019

NAK the "remove redundant field names". This is one of the worst misfeatures in Rust's syntax.

@apoelstra Why? All this redundancy serves no purpose, and I can't see how it helps anything or prevents making a mistake.

@apoelstra

This comment has been minimized.

Copy link
Member

apoelstra commented Aug 5, 2019

It's not redundancy. A field name and a variable are completely different kinds of things, even at the parse level.

Conflating them would make things harder to read/understand, even if it was done without an adhoc separate parse rule that doesn't exist in any other language.

@elichai

This comment has been minimized.

Copy link
Collaborator Author

elichai commented Aug 5, 2019

Yeah I have mixed feelings about that feature.
I think it's only useful for readability when all the fields are the same as the variable name, I don't like it when it's mixed.

@elichai elichai force-pushed the elichai:2019-08-clippy branch from 6e234f7 to ae26c7d Aug 5, 2019
@elichai

This comment has been minimized.

Copy link
Collaborator Author

elichai commented Aug 5, 2019

Dropped the last commit. (implicit field names)

@dpc

This comment has been minimized.

Copy link
Contributor

dpc commented Aug 5, 2019

It's not redundancy. A field name and a variable are completely different kinds of things, even at the parse level.

What do I/you care? It's a problem for the compiler (or its developers). 🤣

The way I think about it: Awfully a lot of times the name of the field is going to be the same as the name of variable used to initialize it. Because they are holding the same thing, right? And they are of the same type, size, performance, flavor and smell.

In essence in:

let foo = make_foo();
struct FooBar
    foo: foo,
}

the foo variable becomes foo field. So why type it twice? Why make humans look at it twice?

The cost: well, there's a new special case rule that might seem odd at first. But after you accept it into your life, it's not actually a problem. The benefit: code looks more compact, less typing, less room for typos.

This case is so common, that I think it's a worthwhile tradeoff. And after a while I kind of feel that foo: someexpression is actually a special case, and FooBar { foo } is the most basic form, it's just it was discovered later in time.

@apoelstra

This comment has been minimized.

Copy link
Member

apoelstra commented Aug 6, 2019

I care because I need to parse the code to read it.

You're assigning the local variable foo to the field with name foo. These two uses of foo are not remotely in the same category, and neither is in the same category as foo: foo which is an assignment.

It's extremely jarring to see something that looks like one of the former, when I'm expecting a field name, then upon seeing a , rather than : I need to mentally compute and substitute the latter.

@dpc

This comment has been minimized.

Copy link
Contributor

dpc commented Aug 6, 2019

I need to mentally compute and substitute the latter.

Give it some time. :)

@apoelstra

This comment has been minimized.

Copy link
Member

apoelstra commented Aug 7, 2019

Fine :). I concede.

@stevenroose

This comment has been minimized.

Copy link
Collaborator

stevenroose commented Aug 13, 2019

Except for those few occasions where a long squashed line is produced, I like all the changes.

@elichai

This comment has been minimized.

Copy link
Collaborator Author

elichai commented Jan 4, 2020

After #349 is merged I'll rebase this and leave only the non controversial ones, (some of these things have theoretic performance effects).

@elichai elichai force-pushed the elichai:2019-08-clippy branch from ae26c7d to 4793975 Jan 5, 2020
@elichai

This comment has been minimized.

Copy link
Collaborator Author

elichai commented Jan 5, 2020

Rebased.
removed all of the controversial changes, the only change that might be controversial IMHO is the use of std::cmp::Ordering, how do people feel about it?

@elichai

This comment has been minimized.

Copy link
Collaborator Author

elichai commented Jan 5, 2020

Oh, another thing that I'm not 100% sure about is the use of the entry API,
because we need the key only if the key is occupied we can't get it by value (looked into changing this is libstd, but that's not possible because of Entry::insert which needs the key by value)

anyway this is a tradeoff between re-looking into the map(BTree is log n for each operation) and cloning the heap allocated key.

@stevenroose

This comment has been minimized.

Copy link
Collaborator

stevenroose commented Jan 7, 2020

anyway this is a tradeoff between re-looking into the map(BTree is log n for each operation) and cloning the heap allocated key.

I think in most cases, cloning is preferred. Keys are usually short, Copy and stack allocated. Like hashes.

@stevenroose

This comment has been minimized.

Copy link
Collaborator

stevenroose commented Jan 7, 2020

ACK 4793975

@apoelstra

This comment has been minimized.

Copy link
Member

apoelstra commented Jan 24, 2020

Needs rebase

elichai added 4 commits Aug 5, 2019
…ash is derived
@elichai elichai force-pushed the elichai:2019-08-clippy branch from 4793975 to 1a83e26 Jan 26, 2020
@@ -131,7 +132,7 @@ impl error::Error for ParseAmountError {


fn is_too_precise(s: &str, precision: usize) -> bool {
s.contains(".") || precision >= s.len() || s.chars().rev().take(precision).any(|d| d != '0')
s.contains('.') || precision >= s.len() || s.chars().rev().take(precision).any(|d| d != '0')

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jan 26, 2020

Collaborator

Does this change the Pattern from a &str to a char? Is that more efficient? Neat, though.

This comment has been minimized.

Copy link
@elichai

elichai Jan 26, 2020

Author Collaborator

asm: https://godbolt.org/z/bLK8Hx (argh, it just does a call to stdlib, and I can't get it to inline it)

The exact code of Pattern is complicated by the intuition is as follows:
if char is less than a byte(ie ascii) so you can use slice::contains which basically iterates over the slice until it finds it. (memchr)
if it's higher then it defaults to the str impl which creates a haystack and does a bunch of logic to calculate UTF-8 bytes points

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.