This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [doc] Add issue tracking for spaceship operator<=> implementation
ClosedPublic

Authored by mumbleskates on Aug 10 2021, 7:30 PM.

Details

Summary

Add issue tracking and assignment for the implementation of P1614R2: The Mothership has Landed.

Diff Detail

Event Timeline

mumbleskates created this revision.Aug 10 2021, 7:30 PM
mumbleskates requested review of this revision.Aug 10 2021, 7:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2021, 7:30 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb accepted this revision.Aug 10 2021, 7:37 PM

More status papers are good. I can't see anything obviously wrong with this and will merge it once it's gotten libc++ approval.

Mordante requested changes to this revision.Aug 10 2021, 10:39 PM
Mordante added subscribers: rarutyun, Mordante.

Thanks for working on this. @rarutyun is also working in this area. I think D103478 should also be in this overview.
The documentation build is failing, can you fix this?

libcxx/docs/Status/SpaceshipIssues.csv
1 ↗(On Diff #365647)

It would be great to have all related papers and issues in this table from the start. It should at the minimum contain P1614.

This revision now requires changes to proceed.Aug 10 2021, 10:39 PM

Updates from feedback, more differentials and papers

Thanks for working on this. @rarutyun is also working in this area. I think D103478 should also be in this overview.
The documentation build is failing, can you fix this?

Thanks! I was able to traverse from here and find a few more in-progress diffs and specifications. I don't feel the need for this to be an unassailably complete accounting of the total state of work at present, I think it's more important that it exist at all first.

It looks like the documentation build failed because there was a promoted warning that that one CSV had no data rows.

libcxx/docs/Status/SpaceshipIssues.csv
1 ↗(On Diff #365647)

Okay I've added that one.

I don't know of many other papers that cover the state of the library standards at the moment, though I did find in putting this together that the <=> operator in [stacktrace.entry.cmp] is not described in the standard; I added P1614R2 and the brand new P2404R0 and a few related LWGs from the C++20 list; let me know if this looks wrong.

mumbleskates updated this revision to Diff 365700.EditedAug 11 2021, 1:23 AM
  • Language standard is fine, stacktrace_entry is specified to model total ordering which is enough
  • add missing quote
Mordante accepted this revision as: Mordante.Aug 11 2021, 8:50 AM
Mordante added a subscriber: Quuxplusone.

Thanks for the update, LGTM! @Quuxplusone since you're also working on the spaceship operator can you have a look and give the libc++ approval?

Quuxplusone accepted this revision.Aug 11 2021, 9:06 AM
Quuxplusone added inline comments.
libcxx/docs/Status/Spaceship.rst
40–41

FWIW, I don't understand what this sentence means at all. I assume it was copied from somewhere else where its meaning is equally unclear.
My guess is that it just means "When there's a TODO in the code, we'll mark it with a TODO comment." Which is pretty obvious.

libcxx/docs/Status/SpaceshipPapers.csv
10

I don't understand what a Status of C++20 means (as opposed to |In Progress|).

libcxx/docs/index.rst
45–49

It'd be nice to a-z these: Format, then Ranges, then Spaceship.

This revision is now accepted and ready to land.Aug 11 2021, 9:06 AM
mumbleskates marked 2 inline comments as done.
  • sort status docs
  • update LWGs status
libcxx/docs/Status/Spaceship.rst
40–41

Yeah this is all lifted from the Ranges one, which has one TODO item in it to add more tests when spaceship operator implementation increases. I can't think of anything that should go here now but it seems reasonable to leave a space for it.

libcxx/docs/Status/SpaceshipPapers.csv
10

That's the literal status of the LWGs; I can go through this and see what parts of this we've done, not done, or haven't affected our lack of doing things yet instead

libcxx/docs/Status/SpaceshipPapers.csv
10

Ah. But on line 2 we've got one |In Progress| already, which I think makes more sense.
The "Status" here should be the status of the feature in re libc++, i.e. blank, |In Progress|, or |Complete|. If people want to know the status of the feature in re Standard adoption and/or progress-through-WG21, they can go look that up elsewhere. (Also, any paper that hasn't yet been at least nearly-adopted for some version of C++ probably shouldn't be in this list at all.)

mumbleskates marked 2 inline comments as done.

Add P2405 and fix quotes

mumbleskates added inline comments.Aug 12 2021, 2:33 PM
libcxx/docs/Status/SpaceshipPapers.csv
10

Yeah this is more up to date now.

I'm not 100% sure what the status is of Justin Bassett's recent papers, but I sense wisdom in them and I'm keeping on the list with no status.

fix typo from copy/pasted paper title

  • Working on tuple, optional, & variant
mumbleskates retitled this revision from Add issue tracking for spaceship operator<=> implementation to [libc++] [doc] Add issue tracking for spaceship operator<=> implementation.Aug 17 2021, 6:31 PM
ldionne added inline comments.Aug 24 2021, 1:17 PM
libcxx/docs/Status/SpaceshipPapers.csv
3–9

I'd like to track those using the regular Cxx20Papers.csv and Cxx20Issues.csv lists instead. SpaceshipProjects.csv should only contain stuff related to https://wg21.link/P1614, otherwise we duplicate information and things become too complicated.

Also, before you mark any LWG issue as "Nothing To Do", you need to explain why that's the case. Normally, this is done as a NFC commit after the fact.

mumbleskates added inline comments.Aug 25 2021, 3:23 AM
libcxx/docs/Status/SpaceshipPapers.csv
3–9

Ok, great. I'm working on cleaning this up further in D108502, we can fix it there.

I'm fine with removing entries here. I originally had this empty, then I went back after feedback to add papers related to the effort. If we don't want duplicates that's fine.

Nothing To Do LWG entries are all individually justifiable because they affect parts of the standard that are not yet implemented, already implemented with the specified changes, or the changes do not affect the implementation.

ldionne added inline comments.Aug 25 2021, 7:16 AM
libcxx/docs/Status/SpaceshipPapers.csv
3–9

Nothing To Do LWG entries are all individually justifiable because they affect parts of the standard that are not yet implemented, already implemented with the specified changes, or the changes do not affect the implementation.

If the relevant part of the standard is not implemented in libc++ yet, then the LWG issue isn't "Nothing to do". We say "Nothing to do" when our implementation already implemented the LWG issue resolution.

I want those "Nothing To Do" entries to be done as a separate review so we can have a discussion on those.