This is an archive of the discontinued LLVM Phabricator instance.

[libc++][modularisation] Split up <concepts> into granular headers.
ClosedPublic

Authored by Quuxplusone on Aug 5 2021, 10:23 AM.

Details

Summary

This is the complete split of <concepts>, with nothing left in the main header — as requested by @cjdb in D107036.

I do still have this as two commits locally, in case @ldionne does want to split out just the stuff required by <compare>. However, this complete split seems fine to me.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Aug 5 2021, 10:23 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptAug 5 2021, 10:23 AM
libcxx/include/__concepts/arithmetic.h
9–10

Missing the trailing _H here (and throughout). Will fix.

Thanks for working on this!

libcxx/include/__concepts/copyable.h
10

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.

libcxx/include/__concepts/equality_comparable.h
10

Shouldn't this file be named equality_comparable_with.h?

libcxx/include/__concepts/same_as.h
26

Can you do a separate commit to remove the _VSTD?

libcxx/include/concepts
1

Please restore // -*- C++ -*-

cjdb requested changes to this revision.Aug 5 2021, 12:51 PM

Looking good, a few things to work out though.

libcxx/include/__concepts/copyable.h
28–33

It's not immediately obvious that movable is in copyable.h. Please either:

  • split up further
  • consolidate with semiregular and regular, and call the file object.h

I'm okay with either approach.

libcxx/include/__concepts/equality_comparable.h
10

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
libcxx/include/__concepts/invocable.h
34–35

It's not immediately clear that regular_invocable lives in invocable.h. Please either:

  • split the file up further
  • consolidate with the other callable concepts and call the header callable.h

I'm indifferent to the chosen approach, but please be consistent in the approach you choose.

libcxx/include/__concepts/relation.h
1

If you choose to split up invocable.h, please also split up this file. The two splices I'm okay with:

  • one concept per file
  • predicate.h and relation.h
libcxx/include/concepts
1

Why?

This revision now requires changes to proceed.Aug 5 2021, 12:51 PM
Quuxplusone marked 3 inline comments as done.Aug 5 2021, 3:59 PM
Quuxplusone added inline comments.
libcxx/include/__concepts/copyable.h
10

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.

libcxx/include/__concepts/equality_comparable.h
10

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>).

libcxx/include/__concepts/invocable.h
34–35

I prefer not to split invocable from regular_invocable, because they are (as far as libc++ is concerned) equivalent.
However, it's good to split this concept from predicate and relation (and the other concepts equivalent to relation) because this file doesn't depend on __boolean_testable but those concepts do.

libcxx/include/__concepts/relation.h
1

I'd prefer not to split further; but I won't object to splitting predicate from relation just for the heck of it. It's kind of intuitive in that concept predicate has a spelling not involving the word relation, whereas all the other equivalent concepts do involve the word relation. So I'll split into predicate.h and relation.h, unless someone stops me first.

libcxx/include/__concepts/same_as.h
26

Sure.

libcxx/include/concepts
1

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.

cjdb added inline comments.Aug 5 2021, 4:39 PM
libcxx/include/__concepts/equality_comparable.h
10

(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).

libcxx/include/__concepts/invocable.h
34–35

:shrug: I guess the same argument that I made for equality_comparable.h vs equality_comparable_with.h could be made here re not splitting further. Though, I'm not sure why you're opposed to __boolean_testable being in this header. Ditto for object.h, but we should keep the conversation in one place.

libcxx/include/__concepts/relation.h
1

Yeah, that's what I meant by predicate.h and relation.h. Thanks!

Quuxplusone marked 10 inline comments as done.

Address review comments.

Quuxplusone added inline comments.Aug 5 2021, 8:55 PM
libcxx/include/__concepts/invocable.h
34–35

__boolean_testable is an ancestor of both equality_comparable and predicate.

cjdb added inline comments.Aug 5 2021, 9:10 PM
libcxx/include/__concepts/invocable.h
34–35

Yes, but __boolean_testable has its own header. I'm trying to work out why you're opposed to combining the all the callables into a single header, and you pointed at __boolean_testable.

Quuxplusone marked an inline comment as done.Aug 5 2021, 9:46 PM
Quuxplusone added inline comments.
libcxx/include/__concepts/invocable.h
34–35

I must not understand what you're asking, because it seems like this is really basic.
My initial PR had:
invocable and regular_invocable go in invocable.h, which does not include boolean_testable.h.
predicate and relation and equivalence_relation and so on go in relation.h, which does include boolean_testable.h.

My current PR splits predicate into predicate.h, purely on the basis of name-spelling, because you asked for it.

Mordante added inline comments.Aug 5 2021, 11:14 PM
libcxx/include/__concepts/copyable.h
10

Spitting sounds good to me too.

libcxx/include/__concepts/equality_comparable.h
10

Indeed I missed the second concept.

libcxx/include/concepts
1

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.

Quuxplusone marked an inline comment as done.

Poke buildkite.

Quuxplusone added inline comments.Aug 6 2021, 8:17 AM
libcxx/include/concepts
133–136

@ldionne: Today, this works:

#include <concepts>
std::true_type f();
// or std::move, std::forward, etc.

What I propose here, will break Modules builds for people who do this.
Notably, our own tests do it in these files:

	modified:   test/std/concepts/concepts.callable/concept.predicate/predicate.compile.pass.cpp
	modified:   test/std/concepts/concepts.callable/concept.predicate/predicate.pass.cpp
	modified:   test/std/concepts/concepts.lang/concept.common/common_with.compile.pass.cpp
	modified:   test/std/concepts/concepts.lang/concept.commonref/common_reference.compile.pass.cpp

Alternatives to choose among:
(1) Since <concepts> is super new (C++20) and we have a chance to "do it right," personally I'm inclined to take this breakage and just fix our four offending tests.
(2) Or, fix the tests but leave the three top-level #includes here. (<__functional/invoke.h> is already a private module, thus safe to remove. <__functional_base> is not in the modulemap at all.)
(3) Or, leave the three top-level #includes and deliberately do not fix the tests, and/or add some new regression tests in test/libcxx/ verifying that libc++'s <concepts> continues to provide true_type, basic_common_reference, move, and so on.

cjdb added inline comments.Aug 6 2021, 9:22 AM
libcxx/include/__concepts/invocable.h
34–35

I prefer not to split invocable from regular_invocable, because they are (as far as libc++ is concerned) equivalent.
However, it's good to split this concept from predicate and relation (and the other concepts equivalent to relation) because this file doesn't depend on __boolean_testable but those concepts do.

You've said that predicate, relation, etc., depending on __boolean_testable is motivation for not consolidating into a single callable.h. You haven't outlined why it's motivation.

It's also occurred to me overnight that @ldionne may want a consolidated header, as he did for <functional>'s function objects. Again, I'm ambivalent on that, but it's something Louis might have a stronger opinion on.

libcxx/include/concepts
1

TIL. Let's keep them in all extensionless headers then.

133–136

I'm inclined to agree with (1). A significantly smaller user-base than our older headers should mean a lot less pain (if any).

If we go down route (3), I'd ask that we fix the existing tests and add a (temporary) regression test.

ldionne added inline comments.Aug 9 2021, 5:49 AM
libcxx/include/concepts
133–136

I would say definitely go with (1) too.

Rebase after IWYU-ing the four offending tests in rG3d2d3b3e7ae4a2fe4.

Quuxplusone added inline comments.Aug 9 2021, 6:45 AM
libcxx/include/__concepts/invocable.h
34–35

@ldionne, could you put this subthread to rest, please?

@ldionne or @cjdb, any final comments on this splitting-up?

cjdb added a comment.Aug 10 2021, 9:03 AM

@ldionne or @cjdb, any final comments on this splitting-up?

Yes. This isn't "obviously correct" and is missing maintainer/two co-approver LGTMs, and there's an unresolved discussion.

libcxx/include/__concepts/invocable.h
34–35

Specifically, this one.

ldionne accepted this revision.Aug 10 2021, 3:36 PM

LGTM, although it also LGTM with the other proposed solution of using callable.h.

libcxx/include/__concepts/invocable.h
34–35

I am neutral on which one of the two following solutions are taken:

  1. callable.h that contains all the "roughly callable" concepts, like invocable, regular_invocable, predicate, etc.
  2. the status quo, where invocable.h defines both invocable and regular_invocable

The one thing I'd much rather avoid is create a new header for just regular_invocable, since it's so trivial.

All those preferences are very, very tiny IMO. I'm basically neutral - I think it simply doesn't matter much because all those solutions make sense to me. I think we should just pick something and move forward with this patch to unblock more important work.

cjdb accepted this revision.Aug 10 2021, 3:42 PM
cjdb added inline comments.
libcxx/include/__concepts/invocable.h
34–35

Looks like there's an answer, but I'd still like an answer for the dependency concern on the record (you can answer that after merging though).

This revision is now accepted and ready to land.Aug 10 2021, 3:42 PM
This revision was landed with ongoing or failed builds.Aug 10 2021, 7:03 PM
This revision was automatically updated to reflect the committed changes.
cjdb added inline comments.Aug 15 2021, 4:20 PM
libcxx/include/__concepts/boolean_testable.h
31

We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made.

libcxx/include/__concepts/invocable.h
34–35

Ping.

libcxx/include/__concepts/swappable.h
66

We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made.

68

We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made.

75

We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made.

77

We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made.

88

We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made.

cjdb added inline comments.Aug 25 2021, 2:48 PM
libcxx/include/__concepts/invocable.h
34–35

I prefer not to split invocable from regular_invocable, because they are (as far as libc++ is concerned) equivalent.
However, it's good to split this concept from predicate and relation (and the other concepts equivalent to relation) because this file doesn't depend on __boolean_testable but those concepts do.

You've said that predicate, relation, etc., depending on __boolean_testable is motivation for not consolidating into a single callable.h. You haven't outlined why it's motivation.

@Quuxplusone this still hasn't been addressed.

@Quuxplusone Indeed, please do not ever modify code in modularisation patches, it's akin to "sneaking in" changes. I'm sure that wasn't your intent, but let's be careful about that.

Please revert to using std::forward -- if we want to use static_cast, we'll do it consistently everywhere.

@Quuxplusone Indeed, please do not ever modify code in modularisation patches, it's akin to "sneaking in" changes. I'm sure that wasn't your intent, but let's be careful about that.
Please revert to using std::forward -- if we want to use static_cast, we'll do it consistently everywhere.

Okay, but I'm going to force you to approve the revert. ;) D108743

libcxx/include/__concepts/invocable.h
34–35

I don't think there's anything more to be said here. You're welcome to keep asking "why?" over and over, but it comes down to software-engineering principles which kind of tie into a whole philosophy. You've already hit bedrock as far as I'm concerned, as soon as we reached "minimize unnecessary dependencies."