This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Move preferred_name declarations into the forward declaring headers and add pmr preferred names
ClosedPublic

Authored by philnik on Oct 12 2022, 4:05 PM.

Details

Summary

We currently define the preferred names in multiple places. basic_string and basic_string_view also have a lot of aliases, which makes the declarations quite long. So let's only add the preferred names in forward-declaring headers to make the implementation more readable and have all the preferred names in one place.

Diff Detail

Event Timeline

philnik created this revision.Oct 12 2022, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2022, 4:05 PM
philnik requested review of this revision.Oct 12 2022, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2022, 4:05 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Oct 13 2022, 9:48 AM
This revision is now accepted and ready to land.Oct 13 2022, 9:48 AM
philnik updated this revision to Diff 468320.Oct 17 2022, 2:22 PM
  • Generate files
philnik updated this revision to Diff 468564.Oct 18 2022, 8:33 AM
  • Rebased
  • Try to fix CI
philnik updated this revision to Diff 468567.Oct 18 2022, 8:34 AM

Fix patch

philnik updated this revision to Diff 468825.Oct 19 2022, 2:33 AM
  • Try to fix CI
philnik updated this revision to Diff 468861.Oct 19 2022, 3:56 AM

Try to fix CI

philnik updated this revision to Diff 471538.Oct 28 2022, 6:55 AM

Rebased onto main

EricWF requested changes to this revision.Oct 28 2022, 10:58 AM
EricWF added a subscriber: EricWF.

This change needs a description explaining it's rational.

This revision now requires changes to proceed.Oct 28 2022, 10:58 AM
EricWF added inline comments.Oct 28 2022, 11:24 AM
libcxx/include/__fwd/string.h
58

Second declaration here.

libcxx/include/regex
6848 ↗(On Diff #471538)

This would have caused anybody including regex without wchar_t to fail to build, right?

How did we not catch that before?

libcxx/include/string
652

Aren't we double defining this typedef?

659

What happened to _LIBCPP_TEMPLATE_VIS?

libcxx/include/string_view
207

Shouldn't this directly include the forward decl header?

philnik edited the summary of this revision. (Show Details)Oct 29 2022, 11:36 AM
philnik updated this revision to Diff 471768.Oct 29 2022, 11:39 AM
philnik marked 5 inline comments as done.

Address comments and try to fix CI

libcxx/include/string
652

yes, good catch!

659

Looks like I removed that accidentally. Is there any way to test for it currently?

libcxx/include/string_view
207

It does, in line 210.

ldionne added inline comments.Oct 31 2022, 10:35 AM
libcxx/include/regex
6848 ↗(On Diff #471538)

The problem here is that we don't have a true C library that does not provide wchar_t. So yes, this would have failed indeed and we would have caught it when building for a platform that truly doesn't provide wchar_t. This is a known hole in our CI coverage, but I think we'd need a custom C library in order to catch this (or an upstream platform that does not provide wchar_t, which is equivalent).

This revision was not accepted when it landed; it landed in state Needs Review.Oct 31 2022, 4:37 PM
This revision was automatically updated to reflect the committed changes.