This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove the synopses
AbandonedPublic

Authored by philnik on Apr 3 2023, 1:56 PM.

Details

Reviewers
ldionne
Mordante
var-const
huixie90
jloser
Group Reviewers
Restricted Project
Summary

The synopsis in the headers mainly exists to track the implementation status of the headers. This is largely obsolete now, since we also track the status of individual papers, and add notes if a paper is not complete yet. In some cases we even add status papers for individual papers if they are large enough. This means that it's become a burden to keep the synopsis up-to-date with relatively little benefit.
If someone wants to look up the synopsis of a header there are also much better online sources for that, making it very unlikely that people look at our synopsis (personally, I exclusively look at it to update it).

Diff Detail

Event Timeline

philnik created this revision.Apr 3 2023, 1:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2023, 1:56 PM
philnik requested review of this revision.Apr 3 2023, 1:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2023, 1:56 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Has this been discussed previously, e.g. on Discord?

Has this been discussed previously, e.g. on Discord?

This came up at https://reviews.llvm.org/D144994?id=502156#inline-1403408. Given that often nothing happens without a patch w.r.t. there kinds of changes I thought I'd create one, but I'm happy to discuss this further if there are any objections to this change.

Has this been discussed previously, e.g. on Discord?

This came up at https://reviews.llvm.org/D144994?id=502156#inline-1403408. Given that often nothing happens without a patch w.r.t. there kinds of changes I thought I'd create one, but I'm happy to discuss this further if there are any objections to this change.

Thanks. I thought there must have been some context there. Personally, I also don't find our synopses very useful. They never feel very reliable since it's so easy for them to go out of sync, they make headers less readable (you have to scroll through a wall of text to get to actual code, sometimes the code is spliced between 2 or more walls of text, e.g. in <map> or <vector>), they make grepping through headers more painful due to extra matches, they add extra boilerplate to every patch that adds new functionality, and so on.

var-const accepted this revision as: var-const.EditedApr 3 2023, 4:37 PM

I'm in favor of this change but we probably need something akin to a consensus to proceed. Thanks for preparing the patch!

jloser accepted this revision.EditedApr 3 2023, 9:30 PM

Big +1 from me. Thanks for doing this.

philnik edited the summary of this revision. (Show Details)Apr 4 2023, 3:22 AM
Mordante requested changes to this revision.Apr 8 2023, 9:53 AM

Has this been discussed previously, e.g. on Discord?

This came up at https://reviews.llvm.org/D144994?id=502156#inline-1403408. Given that often nothing happens without a patch w.r.t. there kinds of changes I thought I'd create one, but I'm happy to discuss this further if there are any objections to this change.

Actually I would have preferred to discuss it first.

I'm also not too happy with the synopsis, but I really think it should be discussed. The commit message mentions that we "and add notes if a paper is not complete yet." which I noticed is not always happening. I feel we are doing that better nowadays, but this is something that is forgotten in patches too. So it still relies on reviewers catching this.

For now I request changes, as request discussion. I haven't looked closely at the patch itself, but some C headers might benefit from a synopsis. Especially due to macros that are not in a using declaration.

This revision now requires changes to proceed.Apr 8 2023, 9:53 AM
philnik abandoned this revision.Sep 14 2023, 9:46 AM