This is an archive of the discontinued LLVM Phabricator instance.

[libc++][doc] Cleanup, normalize, and update projects status docs
ClosedPublic

Authored by ldionne on Aug 20 2021, 6:41 PM.

Details

Summary

Mark the now-done [cmp.result] in spaceship projects as complete; normalize some status markers for papers and projects; fix alignment and line breaks in spaceship projects, add links to standard

Diff Detail

Event Timeline

mumbleskates requested review of this revision.Aug 20 2021, 6:41 PM
mumbleskates created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2021, 6:41 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante requested changes to this revision.Aug 21 2021, 6:07 AM
Mordante added a subscriber: Mordante.

I'm not too fond of patches doing multiple things in one review. It makes finding the real changes a lot harder.

Mark one project in spaceship completed;

Finding this which project that is is quite hard.
I also see some files marked as modified, but without obvious changes.

libcxx/docs/Status/Cxx14.rst
1 ↗(On Diff #367936)

What has changed in this file?

libcxx/docs/Status/FormatIssues.csv
3

Please don't change the status of these entries. There's a patch in review. Unlike the Status/Cxx*.rst pages these are really intended for the developers working on these features. Changing the status makes it harder for me to keep track of the real status.

libcxx/docs/Status/RangesPaper.csv
8 ↗(On Diff #367936)

Why change this? I'm not working on Ranges so don't have a strong opinion. But I assume the authors picked the checkmark since they prefer it that way.

libcxx/docs/Status/SpaceshipProjects.csv
2–8

Thanks for adding these section links!

This revision now requires changes to proceed.Aug 21 2021, 6:07 AM

The specific project that was completed was [cmp.result], the only thing on the list that is marked as complete so far. I can change the differential description to name it specifically. In my head this easily falls into the same bucket as cleaning up formatting, as it is correcting trivial information that is currently wrong.

libcxx/docs/Status/Cxx14.rst
1 ↗(On Diff #367936)

Line endings were CRLF, out of line with other files, for Cxx14-2b and Styles; I can revert all these if that's preferable. Notably this does not affect git blame.

libcxx/docs/Status/FormatIssues.csv
3

Ack

libcxx/docs/Status/RangesPaper.csv
8 ↗(On Diff #367936)

Accessibility; consistency; ability to enter it in without hunting down another check mark and copy-pasting it.

Assuming we want to keep the check marks for some reason it's better to create an entry in Styles.rst like .. |checkmark| unicode:: U+2705 and use that.

This is mostly cjdb's domain, I'll leave it up to him whether he prefers text or a check mark.

  • changes from feedback
cjdb requested changes to this revision.Aug 21 2021, 5:40 PM
cjdb added a subscriber: zoecarver.

I'm with @Mordante on splitting this up into two patches, but I can see why you've combined them.

libcxx/docs/Status/FormatPaper.csv
1–2

It'd be great if you could inject Not started and Unassigned into everything that's blank.

libcxx/docs/Status/RangesPaper.csv
2 ↗(On Diff #367973)

Please leave this as Sentence case (similarly elsewhere).

Alternatively: abandon all changes to this file, as it'll probably get deleted once P0896 is fully implemented. This doc exists mainly to help @zoecarver, @ldionne, and me coordinate who's doing what, and I'm okay with it being a bit different.

8 ↗(On Diff #367936)

No strong preference, as copy/pasting a checkmark is pretty trivial now that it's being used in this file.
As for why we chose checkmarks? No clue. Possibly to make it visually distinct from text-based Not started and In progress, which arguably does have a benefit.

This revision now requires changes to proceed.Aug 21 2021, 5:40 PM
mumbleskates marked 4 inline comments as done.
  • revert RangesPaper.csv
  • Another pass at styling Format statuses
libcxx/docs/Status/FormatPaper.csv
1–2

I added unassigned, |Not Started|, and a styled replacement for |Review|. And some more section links!

libcxx/docs/Status/RangesPaper.csv
2 ↗(On Diff #367973)

haha, i was keeping these titlecased because all the existing examples are titlecased, not because i prefer it.

8 ↗(On Diff #367936)

yeah, we have the whole green background thing going on on the actual page now though which is nice. the check mark on the other hand barely shows up in some of my text editors; i really prefer ascii source files when it's achievable. given your expressed preference against titlecased statuses or whatever i think i'll just revert this file entirely since nothing was added to it other than normalizing the style.

mumbleskates edited the summary of this revision. (Show Details)Aug 21 2021, 6:26 PM
Mordante added inline comments.Aug 23 2021, 9:58 AM
libcxx/docs/Status/Cxx14.rst
1 ↗(On Diff #367936)

I don't object against the change, but it was unclear what was changed and it wasn't mentioned in the patch description. I think it would make sense commit this separately, maybe split the patch after review.

libcxx/docs/Status/FormatIssues.csv
12–13

Since these are on the same format page as the file below. I think here we should also add |Not Started| for consistency.

libcxx/docs/Status/FormatPaper.csv
12

I like adding the wg21 links, but if we add them we should do it consistently in this document.

mumbleskates marked 4 inline comments as done.
  • updates from feedback
libcxx/docs/Status/Cxx14.rst
1 ↗(On Diff #367936)

It is mentioned in the patch description.

Either way is probably fine. As it is this patch is general docs cleanup, and these are the only 5 files with CRLF in all of libcxx.

libcxx/docs/Status/FormatPaper.csv
12

I just added it to the first occurrence of every section. I'll add it to every header if that's what you want.

Mordante accepted this revision as: Mordante.Aug 25 2021, 10:43 AM

LGTM, I still prefer to commit it in separate patches.

libcxx/docs/Status/FormatPaper.csv
12

Ah I didn't notice that. Then for me the current way is fine.

  • Remove line ending fixes from main commit

LGTM, I still prefer to commit it in separate patches.

If you want me to split this up more somehow I can do that too, but it does become increasingly more work for everyone involved.

ldionne accepted this revision.Aug 26 2021, 7:26 AM
ldionne commandeered this revision.Aug 26 2021, 7:28 AM
ldionne edited reviewers, added: mumbleskates; removed: ldionne.
ldionne updated this revision to Diff 368874.Aug 26 2021, 7:30 AM

Rebase onto main. I'll commit this, I reviewed the changes and they look OK to me.

Indeed, it's generally good to try to split functional and non-functional changes from
the start to ease the review process, especially as one is building trust with the reviewers.
But this is an improvement for sure, so I'm shipping this now.

ldionne updated this revision to Diff 368876.Aug 26 2021, 7:33 AM

Fix mistakes when I rebased.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 26 2021, 7:33 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.