This is an archive of the discontinued LLVM Phabricator instance.

[libc++][doc] Use issue labels.
ClosedPublic

Authored by Mordante on Oct 8 2021, 12:27 PM.

Details

Summary

During the review of D111166 I had a private discussion with @ldionne to
avoid the duplication of the C++2b issues in the Ranges and Format
status pages. The main reason for duplicating them is to make it easier to
find them. The title of the paper may not always make it clear to which
project the paper belongs.

This commit removes all LWG-issues from the Ranges and Format status page
and adds labels for these issue in the C++20/C++23 issues list.

A quick scan revealed there are some issues that are missing a label since
they weren't on the ranges issue list. These can be labelled in a separate
commit. In that commit I'll also look for issues for the spaceship operator
and chrono.

Diff Detail

Event Timeline

Mordante created this revision.Oct 8 2021, 12:27 PM
Mordante requested review of this revision.Oct 8 2021, 12:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2021, 12:27 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I like the idea (IIUC) of killing off the separate .csv files for different "projects"' worth of papers, and just using some sort of greppable tags in Cxx20Papers.csv and/or Cxx20Issues.csv.
How does one see how this would render in HTML? Care to render it and post a link to the rendered page somehow?

This can be build by enabling LLVM_ENABLE_SPHINX in cmake and then build the target docs-libcxx-html.
I've posted a screenshot here https://pasteboard.co/xsbuKyAg6isv.png

libcxx/docs/Helpers/Styles.rst
20–23

Looking at the screenshot, I suggest lighter shades; perhaps #E6E6FA, #FAFAD2, #7FFFD4, #B0E0E6, #FAEBD7, #F5F5DC.
https://www.w3schools.com/colors/colors_groups.asp
#CD5C5C and #1E90FF particularly are too dark to comfortably read black-text-on-X-background.

libcxx/docs/Status/Cxx20Issues.csv
304

I suggest no comma between tags.

mumbleskates added inline comments.Oct 10 2021, 9:54 AM
libcxx/docs/Helpers/Styles.rst
20–23

another possibility for background colors is to choose a strong primary with a middling opacity, which avoids becoming unreadable in high-contrast or inverted userstyles.

Mordante marked 2 inline comments as done.Oct 10 2021, 10:07 AM
libcxx/docs/Helpers/Styles.rst
20–23

I looked at your suggestions. I think the first two are too light, I picked a darker shade from the suggested group.

Mordante updated this revision to Diff 378517.Oct 10 2021, 10:09 AM

Addresses review comments.

Mordante added inline comments.Oct 10 2021, 10:16 AM
libcxx/docs/Helpers/Styles.rst
20–23

I noticed this comment after I updated the colors. Do you have suggestions for alternative colors?

This seems like a good ideal to work towards to me, as the organization of these issues is a bit messy.

Is this the final state we want the diff to be? Have we ensured all these papers exist in the other categories? I think I would like a better standin than "1234" for the paper number at the very least.

libcxx/docs/Helpers/Styles.rst
48

These replacements can and should hyperlink to the relevant papers page!

jloser added inline comments.Oct 10 2021, 5:06 PM
libcxx/docs/Helpers/Styles.rst
33

Do we plan on consistently using this? I see we used it in FormatIssues.csv and SpaceshipProjects.csv. Right now in the papers docs, we don't write anything (that, empty implies "Not Started"). Should we fill the empty ones in with "Not Started" or leave it as empty? I don't feel strongly, but I like consistency one way or another. I'm OK if you also see that as out-of-scope for this particular labeling PR.

Mordante added inline comments.Oct 11 2021, 8:57 AM
libcxx/docs/Helpers/Styles.rst
33

I think it would be nice to discuss that, but I think Discord would be a better place. When we want that change I don't mind to make a PR, but should be a separate PR.

48

That could be done is some cases. Chrono is tricky since at the moment all chrono issues are chrono formatting issues. But in the future chono can have its own page; I think there's enough left of C++20 chrono to warrant its own page.

Do we want page links and if so should we make a separate group for |format chrono|?

Quuxplusone requested changes to this revision.Oct 11 2021, 9:12 AM
Quuxplusone added inline comments.
libcxx/docs/Helpers/Styles.rst
48

Maybe I don't understand the context here... I thought the whole point of adding these colored labels was so that we could get rid of the separate "pages." I assumed that the ultimate PR would include

git rm libcxx/docs/Status/SpaceshipProjects.csv
git rm libcxx/docs/Status/SpaceshipPapers.csv
git rm libcxx/docs/Status/FormatPaper.csv
git rm libcxx/docs/Status/RangesPaper.csv

because now, if you want to see what's not-yet-done for a specific feature, you can just Ctrl+F for that feature's colored label on the main Cxx20 or Cxx2b page(s).

It would certainly be extra useful if the HTML page would let you "filter by tag," so that you could view just the [format] issues, or whatever. I don't mind if that CSS magic went into another PR, though.

OTOH, if the point of this PR is not to enable us to git rm those csv files listed above, then I don't understand what the point of this PR is.

This revision now requires changes to proceed.Oct 11 2021, 9:12 AM
Mordante added inline comments.Oct 11 2021, 11:10 AM
libcxx/docs/Helpers/Styles.rst
48

The goal is to only remove the duplication of the LWG-issues on the project status pages. Not the duplication of the papers nor removing those pages entirely.
For example the formatting paper P0645 has taken me the better part of year to implement and some parts are still work-in-progress/not started. Without a status page it will be hard for others to keep track of the status. If somebody wants help with these projects, it's rather easy to see what the status is. That way we can avoid multiple people working on the same feature.

The number of papers is quite small and they are often larger so may take multiple patches to implement them.
There's a huge number of LWG-issues and they are often small and easier the implement.
So that's the main motivation to handle these two differently.

I hope this clarifies the motivation for this patch better.

PS: I intend to remove the Format status page once <format> is implemented. Those who care about the details can find them in the git history.

ldionne requested changes to this revision.Oct 14 2021, 9:31 AM

This direction LGTM and I like the rendered output shown by @Mordante! That way, we should be able to get rid of FormatIssues.csv, RangesIssues.csv & friends.

Regarding FormatPaper.csv, RangesPaper.csv & friends, those are used to break a single paper that is too much work to implement in a single go. So those would still stay after this change. I'm going to request changes so it shows up correctly in the review queue, but let's update the diff and move forward with this!

Mordante updated this revision to Diff 379769.Oct 14 2021, 10:47 AM

Update the paper for its real review.

Mordante marked 4 inline comments as done.Oct 14 2021, 10:51 AM
Mordante added inline comments.
libcxx/docs/Helpers/Styles.rst
48

These replacements can and should hyperlink to the relevant papers page!

For now I haven't done a hyperlink. I think it will give some issues with the papers related to both format and chrono. If we really want them it can be done in a separate commit.

Mordante retitled this revision from [RFC][libc++] Use issue labels. to [libc++][doc] Use issue labels..Oct 14 2021, 10:55 AM
Mordante edited the summary of this revision. (Show Details)
ldionne accepted this revision.Oct 14 2021, 1:27 PM
ldionne added inline comments.
libcxx/docs/Status/RangesIssues.csv
2–30

It looks like we'll need to figure out what to do with those. I think they should just be moved to the appropriate CXXNPapers page and tagged with |ranges|. WDYT? This is fine to do in a separate commit, though.

Quuxplusone accepted this revision.Oct 14 2021, 1:38 PM
Quuxplusone added inline comments.
libcxx/docs/Status/Cxx20Issues.csv
180

Should this say |chrono|?

201

Should this say |chrono|?

210–211

Should these say |format|?

217–218

Should this say |format|?

271

Should this say |chrono|?

288–289

Should this say |chrono|?

libcxx/docs/Status/Cxx2bIssues.csv
45

Should this say |ranges|?

49–51

Should these say |ranges|?

116

Should this say |chrono| |format|? (Incidentally, I'd prefer this alphabetical order over your |format| |chrono|; but not a blocker.)

This revision is now accepted and ready to land.Oct 14 2021, 1:38 PM
Mordante marked an inline comment as done.Oct 15 2021, 8:24 AM

Thanks for the reviews. I'll look at tagging more issues for a separate review.

libcxx/docs/Status/Cxx2bIssues.csv
116

As mentioned in the description I'd like to mark additional papers in a separate commit. Then I also want to look for spaceship related issues to be marked. I prefer to get the initial part in first to avoid getting merge conflicts (I'm sure I already have some).

I put |format| |chrono| in this order since it's logical to me, but I agree alphabetic order is better. I'll fix that prior to committing; one regex replacement and I'm done ;-)

libcxx/docs/Status/RangesIssues.csv
2–30

For implementing format I found it very useful to have the papers still available. So I prefer to keep them. A slightly longer answer is here https://reviews.llvm.org/D111458#inline-1062734. But we can discuss that post commit.

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.
Mordante marked 8 inline comments as done.Oct 16 2021, 3:26 AM

I created a followup marking the issues @Quuxplusone identified and several more in D111935.
.

libcxx/docs/Status/Cxx20Issues.csv
210–211

No it should be |chrono|.

libcxx/docs/Status/Cxx2bIssues.csv
116

This is only |chrono|. The <chrono> header has its own parse function that's unrelated to <format>.