This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [ranges] Add namespace __cpo to ranges::{advance,next,prev}.
ClosedPublic

Authored by Quuxplusone on Jan 3 2022, 5:53 PM.

Details

Summary

The reason for those nested namespaces is explained in D115315:

AIUI, this keeps the CPO's own type from ADL'ing into the std::ranges namespace; e.g. foobar(std::ranges::uninitialized_default_construct) should not consider std::ranges::foobar a candidate, even if std::ranges::foobar is not a CPO itself. Also, of course, consistency (Chesterton's Fence, the economist's hundred-dollar bill): if it were safe to omit the namespace, we'd certainly want to do it everywhere, not just here.

This makes these three niebloids more consistent with the other Ranges niebloids we've already implemented, such as the ranges::begin group and the ranges::uninitialized_default_construct group.

FWIW, we still have three different indentation-and-comment styles among these three groups.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Jan 3 2022, 5:53 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 3 2022, 5:53 PM
ldionne accepted this revision.Jan 4 2022, 7:48 AM

Thanks for making this more consistent with the other niebloids.

This revision is now accepted and ready to land.Jan 4 2022, 7:48 AM
var-const requested changes to this revision.Jan 4 2022, 9:20 AM

The main intention of the patch very much LGTM, but some formatting nitpicks. Please let me know if I'm missing something.

libcxx/include/__iterator/advance.h
74

Formatting nit: I think namespaces aren't supposed to introduce a new level of indentation.

75

It looks like it's using tabs for indentation now, is this intentional?

192

Same nit here: I think this variable declaration shouldn't be indented.

This revision now requires changes to proceed.Jan 4 2022, 9:20 AM
libcxx/include/__iterator/advance.h
74

In general yeah, but this is consistent with most other niebloids/CPOs in libc++ today.
git grep -h struct.__fn libcxx/include/

75

On my browser screen I see little green >> chevrons, indicating that this all bumped over 2 spaces (intentionally); but that's not related to the choice of tabs/spaces.
In fact, CI will be unhappy with tabs, so if CI is green you can assume there are no tabs. Anyway, I'll double-check just to be sure, but I'm 99.99% sure that my editor is too dumb to insert tabs unintentionally. :)

192

Again this is for consistency; and here (unlike above) we're almost 100% consistent with the indentation.
git grep -h 'inline constexpr auto.*__fn' libcxx/include/

var-const added inline comments.Jan 4 2022, 1:18 PM
libcxx/include/__iterator/advance.h
74

I don't see a reason for niebloids to diverge from the style guide, and I don't think we should try to be consistent with things that are incorrectly (IMO) formatted. That would largely defeat the purpose of having a style guide if, once an inconsistent piece of code gets into the code base, it effectively takes precedence over the style guide.

75

Yes, I took >> to mean tabs -- if this isn't the case, please disregard.

ldionne added inline comments.Jan 4 2022, 2:00 PM
libcxx/include/__iterator/advance.h
74

I had missed it, but it does seem more consistent *not* to indent the content of long namespaces since that's what we tend to do elsewhere. Also. it means touching fewer lines, which I guess is good.

Quuxplusone added inline comments.Jan 4 2022, 2:06 PM
libcxx/include/__iterator/advance.h
74

So, leave inline constexpr auto niebloid = indented, but de-dent struct __fn throughout?
If that's the request, I'll do the de-dent in a preliminary commit, and then follow it up with this one (minus the indenting here).

Quuxplusone edited the summary of this revision. (Show Details)

Remove namespace indentation after discussion offline with @var-const. I think this should be uncontroversial now.

As mentioned (now) in the commit message: The ranges::begin group still uses indentation in their namespaces (e.g. all of namespace ranges::__begin is indented 2 spaces). I thought about "fixing" that in this PR too, but decided I don't want to open that can of worms yet. Let's get this landed first.

Quuxplusone added inline comments.Jan 5 2022, 1:12 PM
libcxx/include/__iterator/prev.h
42–43

Oops, meant to add a blank line after line 43 here. Will fix.

@var-const: is this now good to go, as far as you're concerned?

@var-const: is this now good to go, as far as you're concerned?

I've been OOO today, but I'll take a look tomorrow. Thanks for doing the change!

This revision is now accepted and ready to land.Jan 7 2022, 11:01 PM
This revision was landed with ongoing or failed builds.Jan 8 2022, 9:48 AM
This revision was automatically updated to reflect the committed changes.