Skip to content

Implement LWG-3392 ranges::distance() cannot be used on a move-only iterator with a sized sentinel#2421

Merged
StephanTLavavej merged 3 commits into
microsoft:mainfrom
CaseyCarter:lwg3392
Jan 22, 2022
Merged

Implement LWG-3392 ranges::distance() cannot be used on a move-only iterator with a sized sentinel#2421
StephanTLavavej merged 3 commits into
microsoft:mainfrom
CaseyCarter:lwg3392

Conversation

@CaseyCarter
Copy link
Copy Markdown
Contributor

Cleanup conditional noexcept while we're here, and add test coverage.

Fixes #2392

Cleanup conditional `noexcept` while we're here, and add test coverage.

Fixes microsoft#2392
@CaseyCarter CaseyCarter added LWG Library Working Group issue ranges C++20/23 ranges labels Dec 15, 2021
@CaseyCarter CaseyCarter requested a review from a team as a code owner December 15, 2021 23:41
@StephanTLavavej StephanTLavavej changed the title Implement LWG-3392 Implement LWG-3392 ranges::distance() cannot be used on a move-only iterator with a sized sentinel Dec 16, 2021
@StephanTLavavej

This comment has been minimized.

Comment thread stl/inc/xutility Outdated
Comment on lines 1878 to +1880
enum class sized { no, yes };
enum class assign { no, yes };
enum class nothrow { no, yes };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why must these be enum classes instead of simple bools?

It is fun to read nothrow NoThrow = nothrow::no (no, NoThrow!) but noexcept(IsNoThrow) might be clearer...

Pre-existing

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it is much more difficult to reason about what meow<true> does compared to meow<nothrow::no>

Also it is much harder to accidentally pass the wrong thing

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially in this case where it is

trace_iterator<Cat, true, false, true>

Are you sure those are in the right order?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothrow is a terrible name. How many times have I told someone not to put a negative in a name?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you told them not to do it.

Nobody said you could not commit horrible code crimes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I suggest something giving the best of both worlds (in the future, this is pre-existing, also using enum helps a ton):

enum : bool { Throws, NoThrow };
template<bool IsNoThrow = false>
struct meow { ... };

meow<NoThrow> ....

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose you could name the enum if you really want to, but that isn't relevant for clarity, just for preventing type confusion, but type confusion is arguably not a big deal here since really the type is bool for all of them.

@barcharcraz barcharcraz removed their assignment Jan 20, 2022
@StephanTLavavej StephanTLavavej self-assigned this Jan 21, 2022
@StephanTLavavej
Copy link
Copy Markdown
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 59a87cc into microsoft:main Jan 22, 2022
@StephanTLavavej
Copy link
Copy Markdown
Member

Thanks again for implementing all of these ranges LWG issues! 😻 🚀 🪐

@CaseyCarter CaseyCarter deleted the lwg3392 branch August 21, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LWG Library Working Group issue ranges C++20/23 ranges

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LWG-3392 ranges::distance() cannot be used on a move-only iterator with a sized sentinel

4 participants