This is an archive of the discontinued LLVM Phabricator instance.

[libc++][test] Uses qualified std::uint32_t.
ClosedPublic

Authored by Mordante on Mar 7 2023, 11:29 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGda79d6e177c5: [libc++][test] Uses qualified std::uint32_t.
Summary

The module std does not provide c-types in the global namespace. This
means all these types need to be fully qualified. This is a first step
to convert them by using sed.

Since this is an automated conversion other types like uint64_t are kept
as is.

Note that tests in the directory libcxx/test/std/depr/depr.c.headers
should not be converted automatically. This requires manual attention,
there some test require testing uint32_t in the global namespace. These
test should fail when using the std module, and pass when using the
std.compat module.

A similar issue occurs with atomic, atomic_uint32_t is specified as

using atomic_uint32_t = atomic<uint32_t>; // freestanding

So here too we need to keep the name in the global namespace in the
tests.

Diff Detail

Event Timeline

Mordante created this revision.Mar 7 2023, 11:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2023, 11:29 AM
Herald added a subscriber: miyuki. · View Herald Transcript
Mordante requested review of this revision.Mar 7 2023, 11:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2023, 11:29 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante added inline comments.Mar 7 2023, 11:36 AM
libcxx/test/libcxx/type_traits/convert_to_integral.pass.cpp
97

Comments are not really required, but since the conversion will be quite some work I prefer to make no exception for the comments.

libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp
191 ↗(On Diff #503111)

I thought I had reverted this change locally, but seems I didn't.

libcxx/test/std/atomics/types.pass.cpp
145

I didn't fix the alignment:

  • it will correct itself in this case when the other types are done.
  • I really don't want to manually format over 1000 places manually.

Is there a reason you don't qualify all the integer aliases in a single patch? Also, should we maybe start to enforce a few clang-tidy checks on the tests, so we can avoid regressing on this? It's probably not too hard to add a check.

ldionne accepted this revision.Mar 7 2023, 11:42 AM

Is there a reason you don't qualify all the integer aliases in a single patch? Also, should we maybe start to enforce a few clang-tidy checks on the tests, so we can avoid regressing on this? It's probably not too hard to add a check.

Mark and I discussed this offline and we decided to start with a small patch to evaluate how that was going to behave with downstream merging. Other patches will touch things like size_t (so 1000+ files in the test suite). Seeing the reasonable impact of this one, I wouldn't be opposed to doing this for other types like int64_t & friends in this patch, if it keeps the scope similar. But I'm also OK with separate patches.

libcxx/test/std/input.output/iostream.format/output.streams/ostream.formatted/ostream.inserters.arithmetic/minus1.pass.cpp
19

Technically, I think this doesn't need to change since the synopsis in the standard is based on the fact that we're already inside namespace std.

This revision is now accepted and ready to land.Mar 7 2023, 11:42 AM
Mordante updated this revision to Diff 503129.Mar 7 2023, 12:22 PM

Rebased to CI and addresses review comments.

Mordante marked an inline comment as done.Mar 7 2023, 12:26 PM

Thanks for the quick reviews!

Is there a reason you don't qualify all the integer aliases in a single patch? Also, should we maybe start to enforce a few clang-tidy checks on the tests, so we can avoid regressing on this? It's probably not too hard to add a check.

Mark and I discussed this offline and we decided to start with a small patch to evaluate how that was going to behave with downstream merging. Other patches will touch things like size_t (so 1000+ files in the test suite). Seeing the reasonable impact of this one, I wouldn't be opposed to doing this for other types like int64_t & friends in this patch, if it keeps the scope similar. But I'm also OK with separate patches.

As discussed with Louis we don't need to check it. Once we have the module std the CI will check it. So yes we might regress between now and that time, but at least then the changes will be small.

For the first patch I think this is a nice size, for the first step. I feel that doing all other 'u?int(_least|_fast|)\d\d?_t` types can be done in the one patch.

PS I wouldn't mind to clang-tidy our tests too.

libcxx/test/std/input.output/iostream.format/output.streams/ostream.formatted/ostream.inserters.arithmetic/minus1.pass.cpp
19

Maybe I should look at avoiding updating comments. For the synopsis it indeed feels wrong.

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.