[libcxx-commits] [PATCH] D99141: [libcxx] adds `std::incrementable_traits` to <iterator>

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

Ship it with comments applied and CI passing!



================
Comment at: libcxx/include/__config:852
+#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_HAS_NO_CONCEPTS)
+#define _LIBCPP_HAS_NO_RANGES
+#endif
----------------
FWIW, I'm fine with defining this for now, however as soon as we only support compilers that implement concepts, we will get rid of this and instead use just `#if _LIBCPP_STD_VER >= 20` to guard concepts code. Agreed?


================
Comment at: libcxx/include/iterator:461
+template<class _Tp>
+concept __has_integral_minus =
+  !__has_member_difference_type<_Tp> &&
----------------
IMO this name is misleading because you're really checking whether `T` has an integral minus AND no member difference type. I think it would be cleaner to take the `!__has_member_difference_type` out of this helper concept and use it directly below in `incrementable_traits`.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/incrementable_traits.compile.pass.cpp:149
+                                                 cv_qualifier&& noexcept >);   \
+  /**/
+
----------------
cjdb wrote:
> Mordante wrote:
> > cjdb wrote:
> > > zoecarver wrote:
> > > > What's the comment for?
> > > Paging @ldionne. He's not the only Boost dev I've seen do this.
> > Isn't it because line 148 ends with a `\`? Just to avoid when writing code on line 149 to be part of the macro.
> Yeah, but why does line 148 end with `\`?
Good question, I do it as a reflex now. I'm not sure where it came from originally. I know at some point I had a tool to automatically add `\` at the end of macro lines and it would be keyed on `/**/` to stop IIRC, but I'm not sure why we do this historically.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.assoc.types/incrementable.traits/incrementable_traits.compile.pass.cpp:153
+
+#define NO_QUALIFIER
+TEST_POINTER_TO_MEMBER_FUNCTION(empty, NO_QUALIFIER);
----------------
You could use this common idiom to show that you explicitly wanted the macro to expand to nothing, and you didn't forget to escape the line break with `\`:

```
#define NO_QUALIFIER /*nothing*/
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99141/new/

https://reviews.llvm.org/D99141