This is an archive of the discontinued LLVM Phabricator instance.

[libc++][nfc] Use TEST_SAFE_STATIC.
ClosedPublic

Authored by Mordante on Feb 2 2022, 11:04 PM.

Details

Reviewers
philnik
ldionne
Group Reviewers
Restricted Project
Commits
rG5dc1da3e304c: [libc++][nfc] Use TEST_SAFE_STATIC.
Summary

This avoids using an libc++ internal macro in our tests.

Diff Detail

Event Timeline

Mordante requested review of this revision.Feb 2 2022, 11:04 PM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2022, 11:04 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik accepted this revision as: philnik.Feb 3 2022, 4:08 AM
ldionne requested changes to this revision.Feb 3 2022, 5:50 AM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/default.pass.cpp
32–33

It looks like it might be possible to remove the if defined(_LIBCPP_VERSION) now?

Also, I don't quite understand the purpose of the variable itself, but I guess that's an orthogonal question.

This revision now requires changes to proceed.Feb 3 2022, 5:50 AM
Mordante updated this revision to Diff 405673.Feb 3 2022, 9:01 AM
Mordante marked an inline comment as done.

Addresses review comment.

Mordante added inline comments.Feb 3 2022, 9:03 AM
libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/default.pass.cpp
32–33

I haven't looked at what this is exactly used for, I just try to remove the libc++ specific dependencies from the tests.
The attribute enabled is https://clang.llvm.org/docs/AttributeReference.html#require-constant-initialization-constinit-c-20

FWIW, I think this PR is fine as-is, but there also seems to be room for a much larger improvement to how we do (and name) _LIBCPP_SAFE_STATIC at all. Meanwhile, the point of these tests is actually to verify that the relevant ctors are constexpr, so having them use any macro that sometimes expands to "nothing" smells like it's losing us test coverage. If I were doing the followup patch (which maybe I am? let me know :)), I'd:

  • Rename _LIBCPP_SAFE_STATIC to _LIBCPP_CONSTINIT

-Make _LIBCPP_CONSTINIT expand to constinit in C++20 mode, else __attribute__((__require_constant_initialization__)) if supported, else seriously consider making it a hard error (where right now it expands to nothing).

  • Rename TEST_SAFE_STATIC to TEST_CONSTINIT, and make it ​expand to _LIBCPP_CONSTINIT if it's defined, else if C++20 then constinit, else nothing.

FWIW, I think this PR is fine as-is, but there also seems to be room for a much larger improvement to how we do (and name) _LIBCPP_SAFE_STATIC at all. Meanwhile, the point of these tests is actually to verify that the relevant ctors are constexpr, so having them use any macro that sometimes expands to "nothing" smells like it's losing us test coverage. If I were doing the followup patch (which maybe I am? let me know :)), I'd:

  • Rename _LIBCPP_SAFE_STATIC to _LIBCPP_CONSTINIT

-Make _LIBCPP_CONSTINIT expand to constinit in C++20 mode, else __attribute__((__require_constant_initialization__)) if supported, else seriously consider making it a hard error (where right now it expands to nothing).

  • Rename TEST_SAFE_STATIC to TEST_CONSTINIT, and make it ​expand to _LIBCPP_CONSTINIT if it's defined, else if C++20 then constinit, else nothing.

I think this idea sounds fine, and I if you want to clean this up even more, please do.

ldionne accepted this revision.Feb 4 2022, 1:58 PM

I agree with the suggested cleanup too if someone's up for it.

This revision is now accepted and ready to land.Feb 4 2022, 1:58 PM

I agree with the suggested cleanup too if someone's up for it.

I'll take it on, if nobody beats me to it.

This revision was automatically updated to reflect the committed changes.

I agree with the suggested cleanup too if someone's up for it.

I'll take it on, if nobody beats me to it.

I've landed the patch, feel free to make the improvements.