Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable variant P0608R3 in C++17, update LLVM, overhaul variant/any/optional tests #4713

Merged
merged 30 commits into from
Jun 18, 2024

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Jun 4, 2024

Overview

This updates <variant> to make a C++20 behavioral change unconditional, updates our LLVM submodule yet again to pick up small improvements, and finally performs a large overhaul of our LLVM-derived tests for variant, any, and optional. This overhaul picks up newly-added test coverage (e.g. for spaceship comparisons), cleans up test coverage that was unintentionally missing, and splits the extremely large variant test into LLVM-derived and MSVC-specific parts.

Product code

I originally worried about the source-breaking impact of extending this C++20 behavioral change down to C++17. However, libc++ and libstdc++ have implemented this downlevel, the ecosystem has had some time to adapt (for projects that are portable, or compiled with MSVC C++20), and I think the remaining cleanup costs will be manageable.

I thought about guarding this with an escape hatch macro, but I'd like to try just ripping off the bandage. Unlike (for example) header-inclusion escape hatches, supporting both the old and new modes would require the old mode to be tested, and test coverage is what finally pushed me to make this change (as libc++'s tests started assuming this new behavior).

LLVM

transform_llvm.sh

  • Extract script.
    • Use extended regular expressions (sed -E).
    • Use FI[X]ME to avoid any literal occurrences in the repo.
    • Filter out REQUIRES, UNSUPPORTED, and XFAIL, as they would affect the consolidated test.
    • We don't need to use -a in find; logical AND is the default.
    • We can skip nothing_to_do tests for all three.
    • Improve do ... done style.

P0088R3_variant

  • Replace LLVM SOURCES with script output, no manual changes yet.
  • Add namespaces, update run_test() calls.
    • I renamed some of the namespaces to be more consistent.
    • ctor::in_place_type_args::run_test(); is now called in order.
  • Handle std::hash, std::get specializations.
  • Restore EDG and /clr workarounds.
    • With updated "no workaround" arrow comments.
  • Add MSVC workaround, also affects EDG (not yet reported).
    • Also appears in libcxx expected_results.txt:
      std/utilities/variant/variant.relops/three_way.pass.cpp:0 FAIL
      std/utilities/variant/variant.relops/three_way.pass.cpp:1 FAIL
  • Add Clang workarounds, not yet investigated.
    • These also appear in libcxx expected_results.txt:
      # Not analyzed. Clang instantiates BoomOnAnything during template argument substitution.
      std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp:2 FAIL
      # Not analyzed. Clang emits a -Wundefined-inline warning.
      std/utilities/variant/variant.relops/three_way.pass.cpp:2 FAIL
  • Guard relops::three_way, visit::return_type with _HAS_CXX20.
    • We were previously guarding plain visit, which was unnecessary.
  • Guard WG21-P2637R3 Member visit.
  • Use usual_17_matrix.lst and is_permissive.
    • is_permissive allows us to detect the mode without needing the matrix to define macros.
    • This significantly extends test coverage - we previously skipped all of the visit and visit::return_type tests for permissive mode, when only has_visit<BadVariant> was affected.
  • Update instructions to record what we did.

P0220R1_any

  • Cleanup instructions to reduce unnecessary divergence.
  • Replace LLVM SOURCES with script output, no manual changes yet.
  • Restore namespaces, update run_test() calls.
    • I renamed ctor::in_place_type to be more consistent.
    • not_literal::run_test() has been removed.
  • Silence warning C4640: construction of local static object is not thread-safe
  • Add /clr workaround, update instructions to record what we did.

P0220R1_optional

  • Replace LLVM SOURCES with script output, no manual changes yet.
  • Add namespaces, update run_test() calls.
    • I renamed some of the namespaces to be more consistent.
    • The run_test() calls are now exhaustive and ordered; e.g. assign::const_optional_U was previously missing.
  • Handle std::hash specialization.
  • Restore EDG workarounds.
    • With updated "no workaround" arrow comments.
  • Add/restore _HAS_CXX20/_HAS_CXX23 guards.
  • Update instructions to record what we did.

Escape hatches

  • Drop unnecessary deprecation/removal escape hatches.

P0088R3_variant_msvc

  • Extract, part 1. Pure movement, no changes.
  • Extract, part 2. Transfer <msvc_stdlib_force_include.h> preamble.

x64 /std:c++latest compiler memory consumption as reported by /d1reportMemorySummary:

Test Flavor Before After
P0088R3_variant Plain 3.1 GB 2.1 GB
P0088R3_variant_msvc Plain N/A 1.1 GB
P0088R3_variant /analyze 4.9 GB 3.4 GB
P0088R3_variant_msvc /analyze N/A 1.6 GB

This improved total test runtime from 72.5s to 56.4s (1.29x speedup). Presumably this was caused by better parallelism (splitting up long-tail /analyze configurations) and won't significantly impact full test runs.

Use extended regular expressions (`sed -E`).

Use `FI[X]ME` to avoid any literal occurrences in the repo.

Filter out REQUIRES, UNSUPPORTED, and XFAIL, as they would affect the consolidated test.

We don't need to use `-a` in `find`; logical AND is the default.

We can skip `nothing_to_do` tests for all three.

Improve `do ... done` style.
I renamed some of the namespaces to be more consistent.

`ctor::in_place_type_args::run_test();` is now called in order.
With updated "no workaround" arrow comments.
…ted).

Also appears in libcxx expected_results.txt:

std/utilities/variant/variant.relops/three_way.pass.cpp
These also appear in libcxx expected_results.txt:

std/utilities/variant/variant.relops/three_way.pass.cpp
std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
…AS_CXX20`.

We were previously guarding plain visit, which was unnecessary.
This significantly extends test coverage - we previously
skipped all of the visit and visit::return_type tests for permissive mode,
when only `has_visit<BadVariant>` was affected.
I renamed `ctor::in_place_type` to be more consistent.

`not_literal::run_test()` has been removed.
I renamed some of the namespaces to be more consistent.

The run_test() calls are now exhaustive and ordered; e.g. `assign::const_optional_U` was previously missing.
With updated "no workaround" arrow comments.
…nclude.h>` preamble.

x64 `/std:c++latest` compiler memory consumption:

Test                 | Flavor     | Before | After
---------------------|------------|--------|-------
P0088R3_variant      | Plain      | 3.1 GB | 2.1 GB
P0088R3_variant_msvc | Plain      | N/A    | 1.1 GB
P0088R3_variant      | `/analyze` | 4.9 GB | 3.4 GB
P0088R3_variant_msvc | `/analyze` | N/A    | 1.6 GB

This improved total test runtime from 72.5s to 56.4s (1.29x speedup).
Presumably this was caused by better parallelism (splitting up long-tail
`/analyze` configurations) and won't significantly impact full test runs.
@StephanTLavavej StephanTLavavej added enhancement Something can be improved test Related to test code labels Jun 4, 2024
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner June 4, 2024 21:04
@StephanTLavavej