[libcxx-commits] [PATCH] D107584: [libc++][modularisation] Split up <concepts> into granular headers.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__concepts/copyable.h:10
+#ifndef _LIBCPP___CONCEPTS_COPYABLE
+#define _LIBCPP___CONCEPTS_COPYABLE
+
----------------
Quuxplusone wrote:
> Mordante wrote:
> > To be more in line with `constructible.h` how do you feel to name this file `assignable.h`?
> > Mainly since these two headers contain multiple concepts.
> Hmm, I see what you mean. The only library consumers of `concept assignable_from` are `concept movable` and `concept copyable` (and `concept swappable`, which already depends on all of the above; and `concept indirectly_movable_storable`, which already depends on `movable`). So perhaps these three should be moved into the same file.
> 
> However, @cjdb's objection that the name of the file isn't obvious would still apply: `assignable_from`, `movable`, and `copyable` have very similar //semantics// but very different //spellings//. So I'm inclined to just split up `movable.h` from `copyable.h` as the path of least resistance.
> 
> As with the `relation` concepts, I specifically don't want to put `concept copyable` and `concept regular` in the same file because I don't want `copyable` to depend on the comparison traits.
Spitting sounds good to me too.


================
Comment at: libcxx/include/__concepts/equality_comparable.h:10
+#ifndef _LIBCPP___CONCEPTS_EQUALITY_COMPARABLE
+#define _LIBCPP___CONCEPTS_EQUALITY_COMPARABLE
+
----------------
cjdb wrote:
> Quuxplusone wrote:
> > cjdb wrote:
> > > Mordante wrote:
> > > > Shouldn't this file be named `equality_comparable_with.h`?
> > > I think `equality_comaprable` is a better name for two reasons:
> > > 
> > > 1. it's shorter to type and is a catch all for search tools
> > > 2. it captures the notion of equality comparability
> > Data point: This file defines both `concept equality_comparable` and `concept equality_comparable_with` (just as `swappable.h` defines both `swappable` and `swappable_with`). I suspect @Mordante may have missed that. `concept equality_comparable` is just 2 lines long (and surprisingly is //not// just `equality_comparable_with<T, T>`).
> > (and surprisingly is not just `equality_comparable_with<T, T>`).
> 
> The reason for this is because it optimises out the expensive `common_reference_with` check when we know it's true (otherwise we'd just have a binary `equality_comparable`, where `U = T`).
Indeed I missed the second concept.


================
Comment at: libcxx/include/concepts:1
-// -*- C++ -*-
-//===-------------------------- concepts ----------------------------------===//
+//===----------------------------------------------------------------------===//
 //
----------------
Quuxplusone wrote:
> cjdb wrote:
> > Mordante wrote:
> > > Please restore `// -*- C++ -*-`
> > Why?
> Will restore, for consistency with `<vector>` etc.
> I had not noticed that every single header (in the top-level `include/` directory anyway) does this.
> Other than the consistency argument, I agree with @cjdb's "Why?": perhaps we should float a PR to kill this line from //all// libc++ headers, but of course separately from D107584.
This line is used in Emacs and Vim to enable the proper syntax highlighting. When removed these editors show no syntax highlighting on these files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107584