[stdlib] Improved documentation for popLast() & removeLast() in BidirectionalCollection

[louisdh]

Added detailed documentation to popLast() regarding its safety compared to removeLast(),
as well as noting the performance cost popLast() has.

[moiseev]

/cc @dabrahams and @natecook1000

[hooman]

I think a similar note is needed for removeLast() as well.

[louisdh]

@hooman Good point, documentation has now been updated for both popLast() and removeLast().

[dabrahams]

These changes are good, but we shouldn't be claiming popLast is safer. It's only preferable because it's guaranteed to be O(1) if it's available.

[louisdh]

@dabrahams Ok, I removed the safety claim.

[dabrahams]

I'm sorry, but I'm not comfortable with the recommendation either. It seems to me that either removeLast should be considered no less preferable than remove(at:), which can also fail when the collection is empty, or if we really believe its usefulness is so marginal as to be a corner case, we should just tell users to remove(at: index(before: endIndex)).

[moiseev]

@dabrahams Safety claim is perhaps my incorrect wording. It is safer in a sense that the type gives you all the info about the behavior of the function, whereas with removeLast it will trap in runtime. It is not about memory safety, it's about convenience and predictable behavior.

[natecook1000]

I don't want to lose the "must not be empty" sentence. You could rephrase it like this to make a clearer recommendation about when to use popLast():

The collection must not be empty. To remove the last element of a collection that might be empty, use the popLast() method instead.

[natecook1000]

Could we rewrite this as:

Use popLast() to remove the last element of a collection that might be empty. The removeLast() method must be used only on a nonempty collection.

[louisdh]

@natecook1000 Ok, I've updated according to your requested changes.

[moiseev]

Sorry for the back and forth on this, the problem turned out to be deeper than it originally looked =)

The problem with the comment I originally suggested is: both these methods are not necessarily available in all contexts. You can see that removeLast is defined for bidirectional collections and range-replaceable collections, but popLast is only available in bidirectional.

Interestingly, removeLast uses a hook in the protocol (see _customRemoveLast) that returns the last element if it existed, but it also requires the collection to be non-empty.

TL;DR: I think the right thing to do now is simply cross-reference the two methods in the documentation. Pretty much the way you've done it at the time of this writing.

This case clearly identifies the flaw in the design of these methods, solving which might become a part of a follow-up to this proposal one day.

Thanks for working on this, @LouisDhauwe!

[moiseev]

This is sort of weird for a comment only change, but needs to be done.
@swift-ci Please smoke test

[moiseev]

@swift-ci Please smoke test and merge