Skip to content

P0896R4_P1614R2_comparisons/test.cpp: Increase test coverage for MSVC #852

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.

Sign up for GitHub

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

Jump to bottom
Merged
StephanTLavavej merged 7 commits into from
Jun 24, 2020
Merged

P0896R4_P1614R2_comparisons/test.cpp: Increase test coverage for MSVC #852

StephanTLavavej merged 7 commits into from
Jun 24, 2020
+11 −20

Conversation

@SuperWig
Copy link
Contributor

@SuperWig SuperWig commented May 21, 2020 •
edited by CaseyCarter
Loading

Fixes GH-741.

@SuperWig SuperWig requested a review from a team as a code owner May 21, 2020 13:41
SuperWig
SuperWig commented May 21, 2020
View reviewed changes
@SuperWig
Copy link
Contributor Author

SuperWig commented May 21, 2020

Hurrah! One of my PR passes tests!

CaseyCarter
CaseyCarter suggested changes May 21, 2020
View reviewed changes
CaseyCarter
CaseyCarter reviewed May 21, 2020
View reviewed changes
@CaseyCarter CaseyCarter added the enhancement Something can be improved label May 21, 2020
SuperWig and others added 3 commits May 22, 2020 07:57
@SuperWig @CaseyCarter
Update transition comments
3966538 
Co-authored-by: Casey Carter <cartec69@gmail.com>
@SuperWig
Change template to macro
d9ba12c
@SuperWig
Missing )
fe4ab77
@StephanTLavavej StephanTLavavej added test Related to test code and removed enhancement Something can be improved labels May 22, 2020
@SuperWig
Copy link
Contributor Author

SuperWig commented May 24, 2020

Why is that failing? I tested it on my end and it passed (though obviously forgot to do it again when I created a new branch 🙄).

@AlexGuteniev
Copy link
Contributor

AlexGuteniev commented May 24, 2020

Why is that failing?

Because #851 isn't still there?

@SuperWig
Copy link
Contributor Author

SuperWig commented May 24, 2020

Why is that failing?

Because #851 isn't still there?

Ah, right. The PR previously passing made that confusing. Guess that's the

they disagree about when VSO-905257 was fixed.

Part of the issue? Back to 0 passing PRs again... ☹️

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jun 20, 2020

We've updated the CI infrastructure to VS 2019 16.7 Preview 2, so I've pushed a merge.

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jun 23, 2020

Does this completely or partially fix the mentioned bug #741? Because the PR description said "Partially fixes BUG", the keyword detection for "fixes BUG" will resolve it.

@SuperWig
Copy link
Contributor Author

SuperWig commented Jun 23, 2020

I put partially as only VSO-1070391 was fixed and VSO-905257 was partially fixed.

The issue is now DevCom-1044530. There's also VSO-1062601 which was pre-existing as Casey forgot to make note of it.

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Jun 23, 2020

I put partially as only VSO-1070391 was fixed and VSO-905257 was partially fixed.

This PR removes all occurrences of 1070391 and 905257 from the STL; so it completely addresses GH-741. (I've edited the first post to make this clearer.)

The issue is now DevCom-1044530. There's also VSO-1062601 which was pre-existing as Casey forgot to make note of it.

The test still works around these two unfixed compiler bugs which are not mentioned in GH-741.

@SuperWig
Copy link
Contributor Author

SuperWig commented Jun 23, 2020

Yeah I was mainly unsure whether or not creating a new issue for the remaining bugs is worth it vs editing the existing.

@CaseyCarter
Copy link
Contributor

CaseyCarter commented Jun 23, 2020

Yeah I was mainly unsure whether or not creating a new issue for the remaining bugs is worth it vs editing the existing.

Neither, really. We don't make issues to track bug workarounds, we do that directly in the sources with // TRANSITION, meow comments. We do make issues to track workarounds for fixed bugs that can be removed like GH-741, but DevCom-1044530 and VSO-1062601 are still open. I think the work that remains to be done here is to complete review and merge this PR so we can close GH-741.

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jun 23, 2020

Now that the GitHub Issues And Pull Requests extension for VSCode is going to support hover cards in their next release for our GH-741 style citations (see microsoft/vscode-pull-request-github#1783 ), perhaps we should start creating GitHub mirror bugs for compiler bugs we're tracking.

StephanTLavavej
StephanTLavavej approved these changes Jun 23, 2020
View reviewed changes
@StephanTLavavej StephanTLavavej self-assigned this Jun 23, 2020
@StephanTLavavej StephanTLavavej merged commit de3edf2 into microsoft:master Jun 24, 2020
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jun 24, 2020

Thanks again for taking care of this! Removing such workarounds helps the compilers that we depend on in the long run.

@SuperWig SuperWig deleted the ranges-spaceship branch June 24, 2020 08:52
ahanamuk pushed a commit to ahanamuk/STL that referenced this pull request Jul 1, 2020
@SuperWig @CaseyCarter
@StephanTLavavej @ahanamuk
P0896R4_P1614R2_comparisons/test.cpp: Increase test coverage for MSVC (…
ccd934b 
…microsoft#852)

Fixes microsoft#741.

Co-authored-by: Casey Carter <Casey@Carter.net>
Co-authored-by: Stephan T. Lavavej <stl@microsoft.com>
@CaseyCarter CaseyCarter mentioned this pull request Jul 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+1 more reviewer

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

test Related to test code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

P0896R4_P1614R2_comparisons/test.cpp: Remove compiler bug workarounds

4 participants

@SuperWig @AlexGuteniev @StephanTLavavej @CaseyCarter