-
Notifications
You must be signed in to change notification settings - Fork 13.6k
__cpp_lib_three_way_comparison not defined when it should be #73953
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
@llvm/issue-subscribers-c-20 Author: Marcin Kowalczyk (QrczakMK)
https://godbolt.org/z/z67c5EErb
The following code fails the
|
@llvm/issue-subscribers-clang-driver Author: Marcin Kowalczyk (QrczakMK)
https://godbolt.org/z/z67c5EErb
The following code fails the
|
Is this a libc++ issue? https://godbolt.org/z/cP17dKohM |
These are two different macros, one for language support, one for libcxx support. There's a status page for the implementation of the latter. It's essentially complete, though blocked on full chrono and stacktrace support, which is why the macro isn't set yet. Arguably, one could set Footnotes |
The stacktrace parts aren't actually part of P1614. I think it would be reasonable to enable the FTM, since we'll implement the spaceship operator for the chrono parts and never provide all the overloads anyways. You'd want to check |
I don't think we should enable the FTM; our implementation is not complete. The incomplete state of chrono also prevents setting the format FTM. For spaceship we also lack the implementation of (It would have been great if larger features would have more fine-grained FTM like the latest freestanding papers did.) As for the chrono parts I have some local patches which I still need to cleanup and publish. It's mainly a lack of time to get them out. |
This part I don't fully understand TBH. It's obvious that there cannot be formatters for something not implemented yet, so expecting chrono formatters if From the POV, I'd argue that Aside from the missing |
It does not matter whether you or I think this is a good design. It only matters what the Standard says. Per Setting the FTM to that value tells users the paper has been fully implemented in libc++. Also I expect to set the |
The Standard does not have such a requirement -- it simply requires that all of the FTMs are defined. The doc you link is a recommendation for what a non-standards-compliant implementation (such as ours) might wish to do. It is clear that in fact, formatting of chrono types requires both chrono and formatting to be implemented, and I would argue that it's entirely within our power to make a reasonable choice of which macros to define based on which functionality is available. (As long as ultimately, when both are supported, all the expected functionality is there.) It's just a question of what serves users best, not of standards-conformance. I do not believe the intent of the recommendations doc was to completely explore and specify the entire cross-product of potential missing features. Rather, it's just listed in that way only because chrono landed in the spec before format. If chrono had landed after format, then the formatting parts of chrono would've been in the original chrono proposal, and thus part of the chrono feature. In that alternative world, I'd argue that it would be entirely correct to claim full support for the chrono feature, even if we had not implemented the formatting library at all (and thus, couldn't provide chrono formatters). |
I strongly disagree. The formatter paper and a different paper for chrono formatters independently set the FTM to the same value. Whether it was a good idea to do that is a good question, but that has been approved by WG21. |
I don't understand who exactly is being helped by such a strict interpretation. From my POV:
Perhaps I'm not imaginative enough to see what is gained by strict adherence to the letter of the standard, but it seems entirely compliance for compliance's sake to me. The standard is not a person; it does not care, the involved/affected humans do. And IMO any human looking at this could see that:
|
Let me just remind everyone, again, that this has nothing to do with "strict adherence to the letter of the standard"! There is nothing in the standard which deals with which macros your you should define based on having which particular parts of a half-complete implementation. Until we implement all of the features (and define all the FTMs), we are not standards-compliant, period. Whether we define a particular FTM now or later is irrelevant. Yes, some of the people who work on the C++ standard have also written recommendations for when a partially-complete implementation ought to define a given FTM, but these are not part of the standard. If we decide to follow those recommendations or not is unrelated to standards-compliance. |
That's honestly a bit ridiculous. The recommended way to provide
(see Barry's answer to https://stackoverflow.com/questions/65472035/checking-for-three-way-comparison-operator-support-at-compile-time.) However, under libc++, this results in not providing I discovered that by chance because I had actually gotten wrong my feature check, and used
which worked on libc++ perfectly well. Then I "fixed" my check because of configurations which had compiler support but not library support, and as a result, now my library doesn't enable This isn't good. The feature testing macros were supposed to make our lives easier, not create new and exciting configuration problems for us to patch around. |
I have to agree with the various users who commented here. While it is pedantically correct that we shouldn't be defining the FTMs because we don't implement all of C++20, I don't think that is useful for users. I think it is very unlikely that we'd get a bug filed against libc++ because it defines For FTMs that span many parts of the library (like
If I understand correctly, the main problem here is that we don't know what value to set |
It's hard for me to even see the argument that the letter of the standard mandates this behavior. If feature X has macro This essentially means that there aren't two separate macros, but one, with two different names. It just makes no sense at all. |
Strictly speaking, that's correct and that's what WG21 specifies. You can take it up to WG21 if you don't like it. My personal opinion (not libc++'s opinion) is that FTMs were a mis-feature in the first place and checking for compiler versions is really what you need to be doing anyways, since you might need to check for bugs in features, etc. But none of this changes the fact that IMO we shouldn't follow what the Standard says in this specific instance, because it doesn't make sense. We should do what makes FTMs as useful as possible for users. |
As I said earlier: the "letter of the standard" has nothing to say on this subject. The standard only mandates you set all the FTMs to specified values, and implement all the required functionality -- which we have not yet accomplished. It does not say what you should do if you fail to implement the full standard. Of course, we should do something useful and reasonably within the intent behind these FTMs, because that's what's good for our users. IMO, that definitely argues for setting this FTM now, and not waiting for chrono to be completed first. |
I will, if you kindly walk me through how the standard requires it. |
I agree that But I don't know where the claim about std::stacktrace comes from. SD-6 describes the various meanings of The comparison operators for |
Setting it to 202106 implies support for P2216R3 std::format improvements and the older papers, P1361R2 (Text formatting), P0645R10 (Integration of chrono with text formatting) and P1652R1 (Printf corner cases in std::format). If you don't support all of those four, it would be misleading to define it to 202106. You could use something like 201900, or just 1, to mean "not quite everything implied by the value 201905". That would allow users to test if it's defined at all, and if they really care about full support for all C++20 |
Indeed. The whole point of the FTMs is for implementations to document in a programmatically checkable way which parts of a standard they support while support is incomplete. If you only care about full 100% conformance to C++20 (or any other standard), you don't need FTMs. You wait for support to be documented as complete by your implementation, and don't even try to use C++20 features until then. But very very few people work like that. They want to use the supported features as soon as they become available. So the FTMs are intended to enable that.
Right.
The recommendations in SD-6 were written before the macros were added to the standard. The original plan was that the macros would only be documented there, and not in the standard at all. And implementations could choose to follow the SD-6 recommendations because doing so was helpful for their users. But that didn't work, and some implementations said they couldn't possibly follow those recommendations unless they were in the standard itself. So we had to put them in the standard, but the intention was that SD-6 would still be the source of truth and would provide the information for users and implementers to understand what the value YYYYMM for the macro It was always intended that SD-6 would provide more information, in a more useful format, than the fairly meaningless list of macros and values defined in the standard. |
Yes, that's what I did for libstdc++. The chrono formatters (and __cpp_lib_format) were in GCC 13, but chrono::parse was missing from GCC 13. |
Based on a reflector discussion it turns out __cpp_lib_three_way_comparison can be set after implementing [P0768R1] "Library Support for the Spaceship (Comparison) Operator". So it seems we can set the FTM for |
I did an investigation and [P0768R1] "Library Support for the Spaceship (Comparison) Operator" does not add a feature-test macro. This omission was fixed by [P1353R0] "Missing Feature Test Macros". Libc++ has not implemented the latter paper. That explains the current status of this feature-test macro in libc++. I've created #91508 so we can have a better overview of retro-actively added feature-test macros. I've verified the paper and we indeed can implement this feature-test macro. I'll create a patch for that. |
The paper P0768R1 Library Support for the Spaceship (Comparison) Operator did not add a feature-test macro. This omission has been corrected in P1353R0 Missing Feature Test Macros This enables the FTM for P0768R1 Fixes: llvm#73953
The paper P0768R1 Library Support for the Spaceship (Comparison) Operator did not add a feature-test macro. This omission has been corrected in P1353R0 Missing Feature Test Macros This enables the FTM for P0768R1 Fixes: llvm#73953
The paper P0768R1 Library Support for the Spaceship (Comparison) Operator did not add a feature-test macro. This omission has been corrected in P1353R0 Missing Feature Test Macros This enables the FTM for P0768R1 Fixes: #73953 --------- Co-authored-by: S. B. Tam <[email protected]>
This is a workaround for internal builds for the missing three-way comparison feature test macros, see: llvm/llvm-project#73953 `__cpp_lib_three_way_comparison` is intended as the correct way to check for availability of the spaceship operator, but compilers don't set this flag for pedantic reasons. We at least know that spaceship operator **does** work in our codebase. Tests: протестировано CI commit_hash:5f28f3779afc634fd13f482a84f24a9838546273
The paper P0768R1 Library Support for the Spaceship (Comparison) Operator did not add a feature-test macro. This omission has been corrected in P1353R0 Missing Feature Test Macros This enables the FTM for P0768R1 Fixes: llvm/llvm-project#73953 --------- Co-authored-by: S. B. Tam <[email protected]> NOKEYCHECK=True GitOrigin-RevId: c36961bd96583f0d63b78030bd5587b84b1d6cee
https://godbolt.org/z/z67c5EErb
The following code fails the
#error
with-std=c++20 -stdlib=libc++
:The text was updated successfully, but these errors were encountered: