This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Do not define views::all as a CPO since it isn't one

Authored by ldionne on Jul 30 2021, 7:59 AM.


Group Reviewers
Restricted Project

Per, views::all
is a range adaptor object, but not a CPO. We don't need to define it like
a CPO to avoid name collisions with hidden friend functions of the same

Diff Detail

Event Timeline

ldionne requested review of this revision.Jul 30 2021, 7:59 AM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2021, 7:59 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Jul 30 2021, 8:08 AM
Quuxplusone added a subscriber: Quuxplusone.

A range adaptor object is a customization point object...

This revision now requires changes to proceed.Jul 30 2021, 8:08 AM

A range adaptor object is a customization point object...

Hmm, fair enough, I missed that. However, I still stand by the fact that we don't need to use the inline namespace trick because we're not trying to "overload" a name that may already exist in the same namespace. In other words, views::all and ranges::swap are fundamentally different, because views::all is never expected to be customized by users. By the way, that makes me question why range adaptors are considered CPOs - am I missing something or do they really not need this inline namespace dance?

So I can change the commit message to not lie about views::all not being a CPO, but I'd still like someone to explain to me why a function object like views::all needs to be in an inline namespace. I understand why ranges::swap is, but not why views::all is. Anyone?

ldionne abandoned this revision.Jul 30 2021, 10:16 AM

I just talked to Arthur and was convinced that even though we don't *need* views::all to be implemented as a CPO, it's better to do it for consistency. Abandoning.