boostorg / core

Boost Core Utilities
133 stars 83 forks source link

Order the arguments of pointer_in_range #171

Open Quuxplusone opened 5 months ago

Quuxplusone commented 5 months ago

Semantically ordered arguments should be lexically ordered, too. See https://quuxplusone.github.io/blog/2021/05/05/its-clamping-time/

I feel very strongly that the arguments to pointer_in_range — like the arguments to rotate and so on (with the notable, regrettable, exception of std::clamp) — should be given in "semantic order." This means that pointer_in_range(a, b, c) should be true iff a <= b < c.

With the current definition, the programmer has to do a lot of extra brainwork (i.e. consult the documentation) to figure out that pointer_in_range(a, b, c) is true iff a >= b && a < c. (As opposed, to, say, if c >= a && c < b.) Putting the "middle" argument in the "middle" is simply the most natural and self-explanatory choice.

The changes in the test file should speak for themselves: we see that it's only the "middle" argument that's varying, and that the expected return value is false only when the "middle" value incorrectly falls outside the "lower" and "higher" bounds.

I'm sending an email to this effect also to the LEWG reflector, strongly recommending that the same change be made in P3234R0.

Obviously this will be much harder to do after Boost 1.86 ships, so time is of the essence now.

Lastique commented 5 months ago

I feel very strongly that the arguments to pointer_in_range — like the arguments to rotate and so on (with the notable, regrettable, exception of std::clamp) — should be given in "semantic order." This means that pointer_in_range(a, b, c) should be true iff a <= b < c.

I strongly disagree. It is established convention that iterators denoting a range are passed in pairs of arguments, begin and end. Breaking that pair with another argument will be a constant source of confusion and mistakes.

pointer_in_range(a, b, c) meaning a >= b && a < c makes sense if you read it as "pointer a in range [b; c)". This, of course is just an interpretation that helps remembering the order of the arguments, but this is also sort of a convention in itself. std::clamp(a, b, c) is similarly "clamp a between b and c".

Lastique commented 5 months ago

BTW, std::rotate is terrible. I can never remember what it does.

Quuxplusone commented 5 months ago

It is established convention that iterators denoting a range are passed in pairs of arguments, begin and end. Breaking that pair with another argument will be a constant source of confusion and mistakes.

I do agree that if the function were actually pointer_in_range(p, rg), taking a range, that that would be the right signature. But whenever a function takes three arguments that are supposed to be ordered a, b, c, it's really really good to put them lexically in that order.

FWIW, std::rotate takes the range to rotate and the "new beginning" of the range: it "pulls" the new beginning leftward so that that element is now at the beginning of the range. Since the "new beginning" argument is always semantically somewhere between first and last, it is passed lexically between first and last. std::rotate(a, b, c) is defined iff a <= b <= c. So, yeah, it's reasonable to get confused whether std::rotate is "pulling" the elements leftward or "pushing" them rightward — is the middle argument the new beginning or the new end? — but the one thing you can't possibly be confused about is what order the arguments are supposed to be in.

Lastique commented 5 months ago

it's really really good to put them lexically in that order.

Sorry, I don't see why, aside from personal preference. IMO, the least confusing interface is "good", and the least confusing most of the time means the most consistent with other similar interfaces. "a <= b < c" doesn't exist in C++, so trying to introduce it now, and not even in that literal form but a remote shadow of it in the form of the order of arguments, does not make the interface consistent or any less error-prone at all. It just makes that interface an odd ball to stumble upon.

So, yeah, it's reasonable to get confused whether std::rotate is "pulling" the elements leftward or "pushing" them rightward — is the middle argument the new beginning or the new end? — but the one thing you can't possibly be confused about is what order the arguments are supposed to be in.

Given that most if not all std interfaces accepting pairs of iterators accept them as consecutive arguments, the pivot iterator in the middle is confusing, at least at first, until you remember that std::rotate is "special".

pdimov commented 5 months ago

There are at least two reasons to prefer the current argument order. First, in the sentence "the pointer p is in the range [b, e)" the order is p, b, e. (This is consistent with is_base_of.)

Second, the mathematical notation for "p is a member of [b, e)" is p ∈ [b, e).

This is not the same as std::rotate, which takes two consecutive ranges (which also necessarily form a valid range when concatenated.)

glenfe commented 5 months ago

Though the current reflector discussion on LEWG about P3234R0 has focused on span range, I'll update the rationale in R1 to discuss this too.