KEMBAR78
Fix <tuple> on MSVC by wmaxey · Pull Request #56 · NVIDIA/libcudacxx · GitHub
Skip to content
This repository was archived by the owner on Mar 21, 2024. It is now read-only.

Fix <tuple> on MSVC #56

Merged
merged 11 commits into from
Nov 11, 2020
Merged

Fix <tuple> on MSVC #56

merged 11 commits into from
Nov 11, 2020

Conversation

@wmaxey
Copy link
Member

@wmaxey wmaxey commented Oct 29, 2020 •
edited
Loading

decltype is a culprit for a slew of MSVC bugs. About 50 failures have been fixed by hacking the __tuple_sfinae_base trait.

Tests that still need to be addressed:

Failing Tests (8):
    libcu++ :: std/utilities/tuple/tuple.tuple/tuple.apply/apply_extended_types.pass.cpp
    libcu++ :: std/utilities/tuple/tuple.tuple/tuple.cnstr/PR27684_contains_ref_to_incomplete_type.pass.cpp
    libcu++ :: std/utilities/tuple/tuple.tuple/tuple.cnstr/UTypes.pass.cpp
    libcu++ :: std/utilities/tuple/tuple.tuple/tuple.cnstr/alloc.pass.cpp
    libcu++ :: std/utilities/tuple/tuple.tuple/tuple.cnstr/deduct.pass.cpp
    libcu++ :: std/utilities/tuple/tuple.tuple/tuple.cnstr/nothrow_cnstr.pass.cpp
    libcu++ :: std/utilities/tuple/tuple.tuple/tuple.cnstr/test_lazy_sfinae.pass.cpp
    libcu++ :: std/utilities/tuple/tuple.tuple/tuple.helper/tuple_size_structured_bindings.pass.cpp

  Expected Passes    : 70
  Unexpected Failures: 8

@brycelelbach brycelelbach added this to the 1.4.0 milestone Oct 29, 2020
@wmaxey wmaxey force-pushed the bugfix/tuple_msvc branch 2 times, most recently from 00db28e to 48b9300 Compare October 30, 2020 01:05
@wmaxey wmaxey changed the title WIP: Fix <tuple> on MSVC Fix <tuple> on MSVC Oct 30, 2020
@wmaxey wmaxey requested a review from griwes October 30, 2020 01:08
@wmaxey
Copy link
Member Author

wmaxey commented Oct 30, 2020

After a few touch ups MSVC builds clean with only four tests unsupported.

Testing Time: 136.69s
Expected Passes : 76
Unsupported Tests : 4

two of these are actually ICEs or MSVC not supporting something.

Incomplete types for example cannot be used with some intrinsics.

griwes
griwes reviewed Oct 30, 2020
View reviewed changes
.upstream-tests/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/UTypes.pass.cpp

Tup t2(E(0));
static_assert(!test_convertible<Tup, E>(), "");
assert(cuda::std::get<0>(t) == 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...that's embarrassing.

@wmaxey wmaxey force-pushed the bugfix/tuple_msvc branch from 48b9300 to 1085582 Compare October 30, 2020 02:36
@wmaxey wmaxey requested a review from griwes October 30, 2020 06:47
@wmaxey
Copy link
Member Author

wmaxey commented Oct 30, 2020

Internal CI run on CL 29115815

griwes
griwes approved these changes Oct 30, 2020
View reviewed changes
Copy link
Collaborator

@griwes griwes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Have you tested with NVRTC?

@wmaxey
Copy link
Member Author

wmaxey commented Oct 30, 2020

A fresh internal CI run includes an NVRTC pass.

@wmaxey
Copy link
Member Author

wmaxey commented Oct 30, 2020

NVRTC is clean on local builds, however I picked this up after updating to latest available GCC: #67

I'll see if I can submit a fix for this later. It's not related to this change for the time being and breaks on main.

@wmaxey
Copy link
Member Author

wmaxey commented Nov 10, 2020

CI showed that I missed the case for when std::tuple<std::array<type, size>> can be used to create a tuple made up of n-types. I've updated the offending commit and have started re-running CI.

wmaxey
wmaxey commented Nov 11, 2020
View reviewed changes
.upstream-tests/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/deduct.pass.cpp
// into issues with libc++. However GCC accepts this code when compiling
// against libstdc++.
// XFAIL: gcc-5, gcc-6, gcc-7
// XFAIL: gcc-5, gcc-6, gcc-7, gcc-10.2
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See issue #67 about this failure.

It could be resolvable with some investigation since earlier versions are passing.

@wmaxey
Copy link
Member Author

wmaxey commented Nov 11, 2020

CI is clean after marking the last few failures as expected or having fixed them. I'll go ahead and merge this.

CL: 29308251

@wmaxey wmaxey merged commit 721bfd1 into NVIDIA:main Nov 11, 2020
@wmaxey wmaxey mentioned this pull request Dec 3, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@griwes griwes griwes approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@wmaxey @griwes @brycelelbach