Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

is_last naming bikeshed #129

Open
fdwr opened this issue Sep 13, 2023 · 11 comments
Open

is_last naming bikeshed #129

fdwr opened this issue Sep 13, 2023 · 11 comments

Comments

@fdwr
Copy link

fdwr commented Sep 13, 2023

I came here from your recent talk "A Safer Iteration Model for C++ - Tristan Brindle - C++ on Sea 2023", and generally I like it. 😎

The naming of is_last though is confusing because it's not testing whether the current points to the last thing, but rather whether it points after the last thing. The name first is fine for the starting point, but for the stopping point, you really want to know whether the cursor is at the limit (like limits in math, an element one past which is not actually ever reached). I considered possible alternate names 🤔:

  • is_limit (not too long and very clear, that you're at the limit)
  • is_past (coincidentally rhymes with is_last for the one-past 😉)
  • is_done (works, but then it's counterpart flux::done() counterpart doesn't sound so nice)
  • is_stopped (would use pairs flux::start() and flux::stop(), which sounds a bit weird, but not weirder than begin/end)
  • is_complete (hmm, maybe)
  • is_terminated (bit long)
  • is_terminal (maybe)
  • is_final (oops, C++ keyword, so can't use that)

And conversely for the counterpart of flux::first(v):

  • flux::limit(v) (noun works well here)
  • flux::past(v) (clearly evokes the "one past" concept)
  • flux::done(v) (😕 hmm, sounds weird because it sounds like a boolean function rather than returning a one-past sentinel marker)
  • flux::stop(v) (can be a noun like "gradient stop" or a verb, where verb is weird like "end")
  • flux::complete(v) (😕 also sounds like weird like done)
  • flux::terminal(v) (but not to be confused with a command line interface 😅)
  • flux::final(v) (oops, C++ keyword, so can't use that)

And yes, using any of the above counterparts above would not form a pair like first and last do, but then end-exclusive ranges are already an asymmetric concept [ ).

From your talk, where you say "That's called chain in my library, because that's a better name" and "sum, because that's easier to spell than accumulate", I can see you're not strongly attached to existing names. Do any of the above alternate names convey the concept for you nicely, especially limit or past, because if the 62 upvotes from here mean anything, using last is an non-ideal name? Cheers from Seattle.

@tcbrindle
Copy link
Owner

Hi, thanks for filing the issue! As it happens, I was thinking about this as well after I saw that Reddit thread.

In my defence, the name last makes more sense if you think about it as meaning "the last cursor position" rather than "a cursor pointing to the last element" -- since a sequence with N elements has N+1 possible cursor positions. That said, I'm not particularly strongly attached to it, and I can see that there is an ambiguity that isn't ideal.

Back when I started working on the library, cursors were called indices and the sequence functions were called begin_idx(), end_idx() and is_done(). If we went back to using the term "index" rather than "cursor" then I think these function names are pretty good... but the whole reason I changed in the first place was that "index" has strong associations with integers whereas "cursor" makes it clear that in Flux it's a more general concept, so I don't know how I'd feel about changing back now.

Of the names you suggest (thank you for coming up with a list!), I think we actually could use final, because it's not a true keyword and wouldn't conflict with anything. It might be my favourite, in fact. I also like limit as it conveys the past-the-end-ness very nicely.

I'd be keen to get more feedback on this before changing anything so if anyone has any suggestions or preferences then please leave a comment!

@fdwr
Copy link
Author

fdwr commented Sep 13, 2023

if you think about it as meaning "the last cursor position ... That said ... I can see that there is an ambiguity

Yeah, thinking in terms of possible valid index values, last is/was reasonable.

I think we actually could use final

True, it's a contextual keyword. 🤔

I'd be keen to get more feedback

Certainly, leaving it open a while for further comments is wise. Thanks for listening. ⏳👂...

@nuuSolutions
Copy link

I had similar thoughts while watching that talk.

  1. I always considered end() being one past the end a naming smell.
    Testing against end() to see if find() has a result is just crazy imo.
    But one got used to it. With 'end' being kind of an abstract term it seems fine.

  2. 'first' and 'last', however, sound much more concrete, (even more so than front and back) so the abstraction that last is one past the last is much much harder.

  3. If I were to reinvent this, I would probably choose to get rid of is_last() anyway in favor for the iterator simply being invalid. I would also have last() wrap the last valid index. After 25+ years of STL brainwash, nobody would like that, though ;-)

@tcbrindle
Copy link
Owner

I had similar thoughts while watching that talk.

  1. I always considered end() being one past the end a naming smell.
    Testing against end() to see if find() has a result is just crazy imo.

"Did we find what we were looking for or did we reach the end of our search range" isn't all that crazy IMO. And in any case, Flux has flux::contains which just returns a bool if that's what you want.

But one got used to it. With 'end' being kind of an abstract term it seems fine.
2. 'first' and 'last', however, sound much more concrete, (even more so than front and back) so the abstraction that last is one past the last is much much harder.
3. If I were to reinvent this, I would probably choose to get rid of is_last() anyway in favor for the iterator simply being invalid. I would also have last() wrap the last valid index. After 25+ years of STL brainwash, nobody would like that, though ;-)

If you use closed rather than half-open ranges then it means you can't call first() or last() on an empty sequence. So then you need to make is_empty() a customisation point instead of is_last() and have all algorithms check for that, and so you haven't actually simplified anything. This is similar to what D ranges does, which is an interesting model but less powerful and not what I want to do for Flux. Half-open ranges, with their natural treatment of empty sequences, are here to stay I'm afraid -- and it's certainly not "brainwashing".

@nuuSolutions
Copy link

sigh
I was just trying to make the point that a random person would guess (with odds>80%) that - given a list - begin and end refer to the first and last elements. Now what might these odds be if we replaced begin, end with first, last?

@marktsuchida
Copy link

Python uses start and stop for half-open index ranges (as well as other things, though not 100% consistently), and I remember it being much more immediately clear on first sight compared to C++'s begin and end, to the inexperienced programmer that I was at the time. I also do think that it helps to use two words that are a natural pair in English. is_stop() might have an advantage in that it grammatically forces "stop" to be taken to be a noun, which then nudges you to recognize that it is a term with a specific meaning -- but that might just be for me.

Just came to this repo from your wonderfully presented CppNorth talk.

@fdwr
Copy link
Author

fdwr commented Mar 17, 2024

I'd be keen to get more feedback on this before changing anything so if anyone has any suggestions or preferences then please leave a comment!

Rewatching your CppCon talk tonight (Iteration Revisited: A Safer Iteration Model for Cpp - Tristan Brindle) reminded me of this issue. Is half a year enough time for feedback collection? :b

@tcbrindle
Copy link
Owner

After more than a year since this issue was filed, it's probably time for an update!

As part of some wider changes to the Flux concepts, I'm planning to change the names of some of the accessor functions. The proposed new names are as follows:

  • first() becomes start_cur()
  • last() becomes end_cur()
  • inc() becomes inc_cur()
  • is_last() becomes is_in_bounds(), with the boolean reversed

In particular, the last change means that we can provide a safe, bounds checked read_at() when the user provides us with read_at_unchecked(), reducing the number of customisation points that users need to provide. It also allows us to offer a try_read_at() -> optional<T> function which can be useful in some situations.

If anyone feels very strongly that these new names are terrible and I'd be making a big mistake in changing them, please let me know! :)

@nuuSolutions
Copy link

I like the reversed is_in_bounds() idea very much.
The naming is OK, but a simple valid() also seems appropriate for a loop condition as well as for testing the result of a find.

the naming of the _cur things...
begin_cur end_cur would seem more natural to the occasional user coming from STL
in my mind you either go with STL or invent 2 new terms
then again the whole naming dilemma only occurs when manually dealing with iterators, which should be mitigated when using ranges

@fdwr
Copy link
Author

fdwr commented Dec 3, 2024

Is the _cur suffix redundant, given the flux:: namespace primarily deals with cursors already? Otherwise the new naming is okay with me, being clear/unambiguous (now that exclusive last becomes end*). Though like nuuSolutions said, if we're going with end*, then begin* is a standard counterpart. Thanks.

(feel free to close at your discretion once you feel any other folks' feedback is addressed)

@tcbrindle
Copy link
Owner

Is the _cur suffix redundant, given the flux:: namespace primarily deals with cursors already?

These can be member functions as well, so we need to differentiate end_cur() from the traditional end() as classes can have both (and there's also a flux::end() free function that returns a ranges-compatible sentinel).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants