Prefer is_empty to len()

[tamird]

r? @alexcrichton

[alexcrichton]

Hm I think I may prefer to more officially establish a convention in this regard before migrating the codebase. Along those lines I'm curious if others have thoughts on this?

I'm personally pretty ambivalent, I tend to reach for len first just because it's what I'm using 90% of the time elsewhere.

[Diggsey]

"is_empty()" is always going to run in constant time, whereas "len()" may take linear time for some containers.

[Gankra]

vecmap and bitvset still take linear time to answer is_empty if the answer is "true". If it's false they may do better depending on value locations.

[tamird]

@Diggsey good call. @alexcrichton wdyt?

[alexcrichton]

I feel like the most commonly used collections for the compiler, Vec and HashMap, it doesn't matter perf-wise one way or another, but it is indeed a good point that collections may have a more optimized version of is_empty than they do of len.

Overall I'd still prefer a broader consensus for changes like this. Ideally this would be a rustc-specific lint as well, but that seems over the top for something so minor.

[bors]

☔ The latest upstream changes (presumably #23359) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #23796) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #23936) made this pull request unmergeable. Please resolve the merge conflicts.

[gereeter]

I'm definitely inclined to prefer is_empty, both because of the efficiency argument and because if we use len, then is_empty serves no purpose.

[bors]

☔ The latest upstream changes (presumably #23857) made this pull request unmergeable. Please resolve the merge conflicts.

[tamird]

@alexcrichton I think a lint is the way to go here, and probably not a rustc-specific one. I lack the necessary rustc-fu to make this happen though, could you mark this E-mentor?

This lint should probably suggest is_empty() if the following predicates hold:

• fn len(&self) -> usize is called on a receiver that implements an in-scope fn is_empty(&self) -> booland:
  • len()'s return is compared to zero or
  • len()'s return is checked for being less than 1)

WDYT?

[bors]

☔ The latest upstream changes (presumably #23998) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #24177) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #24433) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

Ok I find the comments here pretty convincing, so I think this is fine to go in for now. We'll probably want to discuss a little more if we want a lint for something like this, but it seems beneficial regardless for rustc itself to be consistent.

Thanks for being patient and doing so many rebases @tamird!

@bors: r+ c55ae1d

[bors]

⌛ Testing commit c55ae1d with merge 288809c...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

[lambda-fairy]

@tamird That would make a great addition to https://github.com/Manishearth/rust-clippy

EDIT: Filed as Manishearth/rust-clippy#32

[nikomatsakis]

+1 to is_empty