Details
- Reviewers
ldionne cjdb • Quuxplusone - Group Reviewers
Restricted Project - Commits
- rG58915667d0b9: [libc++][modularisation] Split up <concepts> into granular headers.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/include/__concepts/arithmetic.h | ||
---|---|---|
10–11 | Missing the trailing _H here (and throughout). Will fix. |
Thanks for working on this!
libcxx/include/__concepts/copyable.h | ||
---|---|---|
11 | To be more in line with constructible.h how do you feel to name this file assignable.h? | |
libcxx/include/__concepts/equality_comparable.h | ||
11 | Shouldn't this file be named equality_comparable_with.h? | |
libcxx/include/__concepts/same_as.h | ||
27 | Can you do a separate commit to remove the _VSTD? | |
libcxx/include/concepts | ||
2 | Please restore // -*- C++ -*- |
Looking good, a few things to work out though.
libcxx/include/__concepts/copyable.h | ||
---|---|---|
29–34 | It's not immediately obvious that movable is in copyable.h. Please either:
I'm okay with either approach. | |
libcxx/include/__concepts/equality_comparable.h | ||
11 | I think equality_comaprable is a better name for two reasons:
| |
libcxx/include/__concepts/invocable.h | ||
35–36 | It's not immediately clear that regular_invocable lives in invocable.h. Please either:
I'm indifferent to the chosen approach, but please be consistent in the approach you choose. | |
libcxx/include/__concepts/relation.h | ||
2 | If you choose to split up invocable.h, please also split up this file. The two splices I'm okay with:
| |
libcxx/include/concepts | ||
2 | Why? |
libcxx/include/__concepts/copyable.h | ||
---|---|---|
11 | 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 | ||
11 | 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 | ||
35–36 | I prefer not to split invocable from regular_invocable, because they are (as far as libc++ is concerned) equivalent. | |
libcxx/include/__concepts/relation.h | ||
2 | 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 | ||
27 | Sure. | |
libcxx/include/concepts | ||
2 | Will restore, for consistency with <vector> etc. |
libcxx/include/__concepts/equality_comparable.h | ||
---|---|---|
11 |
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 | ||
35–36 | :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 | ||
2 | Yeah, that's what I meant by predicate.h and relation.h. Thanks! |
libcxx/include/__concepts/invocable.h | ||
---|---|---|
35–36 | __boolean_testable is an ancestor of both equality_comparable and predicate. |
libcxx/include/__concepts/invocable.h | ||
---|---|---|
35–36 | 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. |
libcxx/include/__concepts/invocable.h | ||
---|---|---|
35–36 | I must not understand what you're asking, because it seems like this is really basic. My current PR splits predicate into predicate.h, purely on the basis of name-spelling, because you asked for it. |
libcxx/include/__concepts/copyable.h | ||
---|---|---|
11 | Spitting sounds good to me too. | |
libcxx/include/__concepts/equality_comparable.h | ||
11 | Indeed I missed the second concept. | |
libcxx/include/concepts | ||
2 | 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. |
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. 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: |
libcxx/include/__concepts/invocable.h | ||
---|---|---|
35–36 |
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 | ||
2 | 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. |
libcxx/include/concepts | ||
---|---|---|
133–136 | I would say definitely go with (1) too. |
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 | ||
---|---|---|
35–36 | Specifically, this one. |
LGTM, although it also LGTM with the other proposed solution of using callable.h.
libcxx/include/__concepts/invocable.h | ||
---|---|---|
35–36 | I am neutral on which one of the two following solutions are taken:
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. |
libcxx/include/__concepts/invocable.h | ||
---|---|---|
35–36 | 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). |
libcxx/include/__concepts/boolean_testable.h | ||
---|---|---|
30 | 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 | ||
35–36 | Ping. | |
libcxx/include/__concepts/swappable.h | ||
65 | We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made. | |
67 | We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made. | |
74 | We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made. | |
76 | We have a policy about not modifying source in modularisation patches beyond cut/paste: this change should not have been made. | |
87 | 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 | ||
---|---|---|
35–36 |
@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 | ||
---|---|---|
35–36 | 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." |
Missing the trailing _H here (and throughout). Will fix.