Page MenuHomePhabricator

[ADT] Add SmallVector::pop_back_n
ClosedPublic

Authored by njames93 on Nov 1 2020, 5:28 PM.

Details

Summary

Adds a method called pop_back_n to SmallVector.
This is more readable and less error prone than the alternatives of using

Vector.resize(Vector.size() - N);
Vector.erase(Vector.end() - N, Vector.end());
for (unsigned I = 0;I<N;++I) Vector.pop_back();

Diff Detail

Event Timeline

njames93 created this revision.Nov 1 2020, 5:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2020, 5:28 PM
njames93 requested review of this revision.Nov 1 2020, 5:28 PM

More expressive in what way(s)? I hesitate a little to add more surface area compared to the standard library as it adds a bit of friction when moving between different containers. (not an outright "never" but "might be a bit higher bar than it would otherwise be")
Could/should this use naming conventions from StringPiece? (there's various drop_n, etc)

More expressive in what way(s)?

Vector.resize(Vector.size() - 2); This takes a little longer to reason and its more error prone. If a typo or bad refactor caused the call to size to be on a different container which just happens to have a size method, that could easily go unnoticed
Vector.pop_back_n(2) is very easy to see what its doing and it removes the aforementioned risk.

Could/should this use naming conventions from StringPiece? (there's various drop_n, etc)

I'm easy on the name, don't really know where pop_back_n came from to be honest. I considered what ArrayRef and StringRef use, drop_back, but that didn't sound quite as nice.
Overloading pop_back could work, but I don't know if anything would be gained from that.

dblaikie accepted this revision.Nov 2 2020, 9:53 AM

Fair enough - might be worth updating the commit message to clarify what's meant by "expressive" (don't have a perfect suggestion, perhaps "more self-documenting"? "expressive" makes me think/read it as though this new API provides more expressive power, as though it can do something the other API cannot, for instance)

This revision is now accepted and ready to land.Nov 2 2020, 9:53 AM
njames93 edited the summary of this revision. (Show Details)Nov 3 2020, 6:42 AM
njames93 edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Nov 3 2020, 6:57 AM
This revision was automatically updated to reflect the committed changes.