Page MenuHomePhabricator

[STLExtras] Add a two argument form of make_early_inc_range
Needs ReviewPublic

Authored by foad on Apr 30 2021, 5:30 AM.

Details

Summary

It seems convenient to me to be able to use make_early_inc_range as a
direct replacement for make_range, so you can change:

make_range(X, Y)

into:

make_early_inc_range(X, Y)

instead of:

make_early_inc_range(make_range(X, Y))

Diff Detail

Event Timeline

foad created this revision.Apr 30 2021, 5:30 AM
foad requested review of this revision.Apr 30 2021, 5:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2021, 5:30 AM
foad added a comment.Apr 30 2021, 5:31 AM

This is an RFC. I'm not really sure if it's worth doing, it just seemed natural to me that make_range and make_early_inc_range could be used in the same way.

Seems fine to me in principle. Wait for others to weigh in though. @rriddle @dexonsmith

Seems fine to me in principle. Wait for others to weigh in though. @rriddle @dexonsmith

I agree, this seems nice / uncontroversial. @dblaikie reviews much of the range stuff, might be worth giving him some time to come across it too.

Might be worth adding a small unit test, besides the examples in code, although I'm not sure what we usually do for simple wrappers like this...

This is an RFC. I'm not really sure if it's worth doing, it just seemed natural to me that make_range and make_early_inc_range could be used in the same way.

Agreed they could be used in this way (existence proofs are the callsites you've updated in this patch) - but I'm not sure they need a wrapper that does both together. I rather like the orthogonality/composability of things. But I wouldn't insist this never be done - just vague preference to leaving them as separate things that callers can compose.

Might be worth adding a small unit test, besides the examples in code, although I'm not sure what we usually do for simple wrappers like this...

Yeah, I think we're pretty hit-or-miss on testing such small things. I'm about where you are on this (if this patch is committed) - might be nice to have a unit test, but not the end of the world in any case.

A few extra words: Generally I'd hope we move towards fewer uses of make_range (& mostly in the implementation of container-like things, rather than in random code handling iterators, like in the two places touched in this patch) and more towards having ranges around for a while to begin with.

For instance, the second piece of code, in LoopInterchange, might be nice if we had a "drop last" operation that operated on a range and this code became:

make_early_inc_range(drop_last(*InnerLoopPreHeader))
foad added a comment.May 1 2021, 7:19 AM

This is an RFC. I'm not really sure if it's worth doing, it just seemed natural to me that make_range and make_early_inc_range could be used in the same way.

Agreed they could be used in this way (existence proofs are the callsites you've updated in this patch) - but I'm not sure they need a wrapper that does both together. I rather like the orthogonality/composability of things. But I wouldn't insist this never be done - just vague preference to leaving them as separate things that callers can compose.

I'd be fine with that except that the name make_<something>_range strongly suggests to me that it should behave in the same kind of way as make_range, just making some specific kind of range instead.

This is an RFC. I'm not really sure if it's worth doing, it just seemed natural to me that make_range and make_early_inc_range could be used in the same way.

Agreed they could be used in this way (existence proofs are the callsites you've updated in this patch) - but I'm not sure they need a wrapper that does both together. I rather like the orthogonality/composability of things. But I wouldn't insist this never be done - just vague preference to leaving them as separate things that callers can compose.

I'd be fine with that except that the name make_<something>_range strongly suggests to me that it should behave in the same kind of way as make_range, just making some specific kind of range instead.

Oh, sure - though I think the lesson to take from that is that the abstraction is correct (because composability/orthogonality is good) and the name is less than ideal. Totally open to naming suggestions/renaming this thing.