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

Fix test_permutations() in P0202R3_constexpr_algorithm_and_exchange #2137

Merged

Conversation

StephanTLavavej
Copy link
Member

First, this simplifies the P0202R3_constexpr_algorithm_and_exchange test by avoiding a huge unnecessary && chain. When performing "dual mode" testing at runtime and compiletime, only the top-level function needs to be constexpr bool returning true (even that isn't strictly necessary, but it's convenient). The helper functions can be constexpr void; any failures will still be detected by failed asserts.

Second, this adds a missing call to test_permutations().

Third, this fixes test_permutations() which never worked.

  • Regardless of whether DevCom-892153 is a compiler bug, we don't need <array>; we can use a built-in multidimensional array with a size_t cursor.
  • int buff[] = {1, 2, 3, 4}; needs to be int buff[] = {10, 20, 30, 40};.
  • This says constexpr int expected[24][4] instead of constexpr int expected[][4] for clarity. I didn't bother to extract 24 as a magic number, although I could do that if anyone wants.
  • After cycling through next_permutation(), this verifies that buff has wrapped around to be equal to expected[0].
  • Previously, after assert(!prev_permutation(begin(buff), end(buff)));, this dereferenced cursor == end(expected). Now we reset cursor = 23;.
  • After cycling through prev_permutation(), this attempted to verify assert(is_sorted(begin(buff), end(buff)));. That's incorrect - the array is in ascending order after cycling through next_permutation(), but in descending order after cycling through prev_permutation(). I have replaced this with stricter tests: we inspect both cursor and verify that we have exactly expected[23]'s content (this is stricter than verifying is_sorted() with greater<>{}).
    • Technically, this final check of the array's contents is redundant (we performed it during the first iteration of the do/while loop after assert(!prev_permutation(begin(buff), end(buff)));) but it seems reasonable to maintain consistency with the next_permutation() code above.

@StephanTLavavej StephanTLavavej added the test Related to test code label Aug 18, 2021
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner August 18, 2021 20:29
Copy link
Contributor

@AdamBucior AdamBucior left a comment

Choose a reason for hiding this comment

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

LGTM (although I already fixed test_permutations in #2043).

@StephanTLavavej StephanTLavavej added this to Initial Review in Code Reviews Aug 18, 2021
Co-authored-by: Adam Bucior <35536269+AdamBucior@users.noreply.github.com>
@StephanTLavavej
Copy link
Member Author

@AdamBucior Ah, my apologies for partially duplicating your work! I noticed that this test was incorrect last night (it came to my attention because it was suppressing clang-format) and didn't realize that your PR had already fixed it.

I believe that our changes can be combined - I'd like to proceed with this PR's more extensive overhaul, and then #2043 can add the is_permutation() checks (both for (const auto& arr : expected) near the top, and the specific examples near the bottom). They'll need manual merge conflict resolution but I believe they can be completed in either order.

}

constexpr bool test_permutations() {
int buff[] = {1, 2, 3, 4};
Copy link
Member

Choose a reason for hiding this comment

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

<meme>I don't always fail to run broken STL test cases, but when I do ...</meme> (No change requested.)

@CaseyCarter CaseyCarter moved this from Initial Review to Final Review in Code Reviews Aug 19, 2021
@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Aug 20, 2021
@StephanTLavavej StephanTLavavej self-assigned this Aug 26, 2021
@StephanTLavavej
Copy link
Member Author

I'm mirroring this to an MSVC-internal PR. Please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 942e5f2 into microsoft:main Aug 27, 2021
Code Reviews automation moved this from Ready To Merge to Done Aug 27, 2021
@StephanTLavavej StephanTLavavej deleted the cleanups2-fix-test-permutations branch August 27, 2021 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Related to test code
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants