This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] variant: test coverage for LWG2904 and P0602
ClosedPublic

Authored by CaseyCarter on Apr 29 2017, 7:25 AM.

Details

Summary
  • Incorporate the variant trivial/constexpr copy/move construct/assign tests from the libcxx tree into the std tree.
  • Update tests for LWG2904, conditional on !defined(_LIBCPP_VERSION) (These all have nicely greppable LWG2904 comments.)
  • Workaround C1XX's buggy __is_trivially_copyable in the variant tests.

All tests pass on both x86 VC++ / x64 clang-4 libc++.

Diff Detail

Event Timeline

CaseyCarter created this revision.Apr 29 2017, 7:25 AM

Fix a weird corner case in variant's move assignment triviality test.

CaseyCarter updated this revision to Diff 97367.May 1 2017, 4:48 PM

Fix missing indent in msvc_stdlib_force_include.hpp

CaseyCarter updated this revision to Diff 97768.May 3 2017, 9:14 PM

Fix typos in test/std/utilities/variant/variant.variant/variant.assign/move.pass.cpp.

EricWF edited edge metadata.May 4 2017, 1:15 AM

Could you commit all the changes *except* the trivial extensions tests, and then update this to contain only that?

CaseyCarter updated this revision to Diff 97834.May 4 2017, 8:53 AM

The constexpr variant tests were enabled separate commit (https://reviews.llvm.org/rL302158). This differential now contains only the fine-grained SMF triviality extension test.

mpark edited edge metadata.EditedMay 23 2017, 6:03 PM

@CaseyCarter: Do these changes pass with the current version?
Also, have you seen the tests in test/libcxx/utilities/variant/variant.variant?
If yes, do those tests and the ones in this diff overlap at all?
Curious as to how we should merge them.

@CaseyCarter: Do these changes pass with the current version?
Also, have you seen the tests in test/libcxx/utilities/variant/variant.variant?
If yes, do those tests and the ones in this diff overlap at all?
Curious as to how we should merge them.

Disclaimer: I know nothing about the libc++ <variant>, and I didn't realize there were tests in the libcxx tree for variant.

These tests technically "pass" with the current version since they do nothing when testing libc++ - they're currently only enabled when testing the MSVC standard library. I hadn't bothered to try them with libc++ variant before, but when I do they all pass except for triviality_test of copy/move assignment with NoCopy as an alternative. Those results indicate to me that you already implement fine-grained SMF triviality, and the discrepancy with NoCopy is probably due to the lack of LWG2904.

Taking a look at the libcxx tree tests:

  • There are SMF sfinae tests in these for (what is now) standard behavior that should be moved into the std tree tests.
  • Your test_XXX_basic tests are effectively testing the same behavior as these but with "is constexpr" as a proxy for "is trivial", and using more demonstrative code whereas mine only tests traits.

I think I'd like to integrate those tests into the std tree, if that's ok with you.

Yes, you're right that fine-grained SMF triviality is implemented but LWG 2904 is not yet.
I would love it if you can integrate the currently libcxx tests into std tests!

Thank you :)

EricWF added inline comments.May 25 2017, 12:40 AM
test/std/utilities/variant/variant.variant/variant.assign/copy.pass.cpp
603

triviality_test should also compare to an explicitly specified expected result, along with checking for consistency between variant and its input types.

test/std/utilities/variant/variant.variant/variant.ctor/copy.pass.cpp
265

These tests pass with libc++ too, so please add || defined(_LIBCPP_VERSION)

test/std/utilities/variant/variant.variant/variant.ctor/move.pass.cpp
336

These tests pass with libc++ too, so please add || defined(_LIBCPP_VERSION)

CaseyCarter retitled this revision from [libcxx] [test] variant: test coverage for P0602 extension to [libcxx] [test] variant: test coverage for LWG2904 and P0602.
CaseyCarter edited the summary of this revision. (Show Details)

Massive update: Move the libcxx tree tests into std, merge my LWG2904 differential.

mpark accepted this revision.Jun 6 2017, 3:55 PM
mpark added a child revision: D33965: Implement LWG 2904..
This revision is now accepted and ready to land.Jun 6 2017, 3:56 PM
CaseyCarter closed this revision.Jun 6 2017, 5:08 PM
test/libcxx/utilities/variant/variant.variant/variant.assign/move.pass.cpp