This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Qualifies size_t.
ClosedPublic

Authored by Mordante on Mar 14 2023, 1:29 PM.

Details

Reviewers
ldionne
EricWF
philnik
Group Reviewers
Restricted Project
Commits
rGfb855eb941b6: [libc++] Qualifies size_t.
Summary

This has been done using the following command

find libcxx/test -type f -exec perl -pi -e 's|^([^/]+?)((?<!::)size_t)|\1std::\2|' \{} \;

And manually removed some false positives in std/depr/depr.c.headers.

The std module doesn't export ::size_t, this is a preparation for that module.

Diff Detail

Event Timeline

Mordante created this revision.Mar 14 2023, 1:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 1:29 PM
Mordante updated this revision to Diff 505519.Mar 15 2023, 8:56 AM

CI fixes.

Mordante updated this revision to Diff 505541.Mar 15 2023, 9:58 AM

Trigger CI.

Mordante published this revision for review.Mar 17 2023, 10:01 AM
Mordante added a reviewer: ldionne.
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2023, 10:01 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
EricWF requested changes to this revision.Mar 17 2023, 2:31 PM
EricWF added a subscriber: EricWF.

Why? If we've done everything correctly, there should be no difference between the two type? What's the motivation for this?

Change descriptions must include a motivation.

This revision now requires changes to proceed.Mar 17 2023, 2:31 PM
philnik accepted this revision.Mar 17 2023, 2:42 PM
Mordante edited the summary of this revision. (Show Details)Mar 18 2023, 5:42 AM

Why? If we've done everything correctly, there should be no difference between the two type? What's the motivation for this?

Change descriptions must include a motivation.

Good point, I see I forgot that. I've updated the commit message. This is part of a series of patches to remove names from the global namespace. These names are not available in C++23's std module.

ldionne accepted this revision.Mar 20 2023, 11:17 AM

I just went through the patch and it LGTM. The table of contents was really useful here.

LGTM but please give a bit of time for @EricWF to raise any concerns if he still has any.

EricWF accepted this revision.Mar 20 2023, 1:56 PM

Thanks for updating the description. With rational, this makes a ton of sense.

So we've changed a bunch of tests here. Do we have a plan for making these tests actually run with true-modules as well as with include?
(This shouldn't block this change, but I want to know where we're headed).

This revision is now accepted and ready to land.Mar 20 2023, 1:56 PM

So we've changed a bunch of tests here. Do we have a plan for making these tests actually run with true-modules as well as with include?
(This shouldn't block this change, but I want to know where we're headed).

We indeed want to run them with true modules. This should also prevent regressions where ::size_t will be reintroduced in the test suite. (Without regression test it will be a matter of time.)
We have not yet decided on an exact plan yet. I have a script (https://reviews.llvm.org/D144994#change-Wk5BJhOsPqr9) that can convert our current tests to use imports. Some people are not happy with committing the result in tree, for now the script is intended be used in the CI to convert the tests.

This revision was landed with ongoing or failed builds.Mar 21 2023, 9:41 AM
This revision was automatically updated to reflect the committed changes.