core icon indicating copy to clipboard operation
core copied to clipboard

Order the arguments of pointer_in_range

Open Quuxplusone opened this issue 10 months ago • 6 comments

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.

Quuxplusone avatar Apr 17 '24 20:04 Quuxplusone

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 avatar Apr 17 '24 21:04 Lastique

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

Lastique avatar Apr 17 '24 22:04 Lastique

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.

Quuxplusone avatar Apr 17 '24 22:04 Quuxplusone

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".

Lastique avatar Apr 17 '24 22:04 Lastique

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.)

pdimov avatar Apr 18 '24 07:04 pdimov

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.

glenfe avatar Apr 18 '24 09:04 glenfe