Skip to content

Conversation

@miscco
Copy link
Contributor

@miscco miscco commented Feb 7, 2020

Description

This addresses parts of #50 now that is_constant_evaluated has been implemented and we can proceed. I did not add the feature test macros fo functional and array as the larger parts are still open.

Note that there was one prexisting bug with the constructor of insert_iterator that missed the explicit keyword

Also tuple is hell...

Fixes #203.

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

  • Identifiers in product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
  • The STL builds successfully and all tests have passed (must be manually
    verified by an STL maintainer before automated testing is enabled on GitHub,
    leave this unchecked for initial submission).
  • These changes introduce no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate
    or trivially copyable, etc.).
  • These changes were written from scratch using only this repository,
    the C++ Working Draft (including any cited standards), other WG21 papers
    (excluding reference implementations outside of proposed standard wording),
    and LWG issues as reference material. If they were derived from a project
    that's already listed in NOTICE.txt, that's fine, but please mention it.
    If they were derived from any other project (including Boost and libc++,
    which are not yet listed in NOTICE.txt), you must mention it here,
    so we can determine whether the license is compatible and what else needs
    to be done.

@miscco miscco requested a review from a team as a code owner February 7, 2020 14:37
@StephanTLavavej StephanTLavavej added the cxx20 C++20 feature label Feb 7, 2020
@BillyONeal
Copy link
Member

@miscco Could we trouble you to add tests for these things now that we have tests in the repo?

@miscco
Copy link
Contributor Author

miscco commented Feb 27, 2020

yes thats on my list for the coming days

@miscco
Copy link
Contributor Author

miscco commented Mar 3, 2020

Lets write some tests...

:: cries in tuple ::

On a more serious note I dont think I can write the tests fo the *_inserter as we do not have constexpr containers with push_back

@BillyONeal
Copy link
Member

:: cries in tuple ::

Yeah, that's the thing with this change and why we aren't already shipping it. A few lines of product code but a lot of tests are necessary. (That's why we haven't jumped on / tried to merge this now that we have tests here on GH)

On a more serious note I dont think I can write the tests fo the *_inserter as we do not have constexpr containers with push_back

You can write something like this pseudocode:

struct constexpr_container {
    using value_type = int;

    array<5> buffer{};
    size_t selected = 0;
    void push_back(int i) { buffer[selected++] = i; }
};

constexpr bool test() {
    constexpr_container c;
    auto tested = back_inserter(c);
    *tested++ = 42;
    *tested++ = 1729;
    *tested++ = 1234;
    assert(c.buffer == {42, 1729, 1234, 0, 0});
    return true;
}

int main() {
assert(test());
static_assert(test());
}

@miscco
Copy link
Contributor Author

miscco commented Mar 3, 2020

@BillyONeal Thanks a lot for the code example. I will try it out, the other tests are hopefully finished. I went with all those tuple constructors on cppreference

@miscco
Copy link
Contributor Author

miscco commented Mar 3, 2020

I went through all the changes and even found some bugs.

Who would have thought that tests do actually help.

@BillyONeal
Copy link
Member

And people wonder why I'm so adamant about "no product changes without corresponding tests"

@miscco
Copy link
Contributor Author

miscco commented Mar 3, 2020

Do you have no faith?

I am a random person from the internet, what could go wrong?

@BillyONeal
Copy link
Member

None.

@BillyONeal
Copy link
Member

I am a random person from the internet, what could go wrong?

It has nothing to do with you being a random person on the internet, only the "person" part. The only person I've seen with a brain compiler with a near 0% failure rate is Stephan ... and even then not close enough to 0 that I don't want tests :)

@miscco miscco mentioned this pull request Mar 6, 2020
4 tasks
@miscco miscco force-pushed the misc_constexpr branch 2 times, most recently from b30d3e3 to b44c4b8 Compare March 9, 2020 11:33
@miscco
Copy link
Contributor Author

miscco commented Mar 9, 2020

I rebased on #589

@miscco
Copy link
Contributor Author

miscco commented Mar 10, 2020

Rebased again due to whatever conflict there was

@CaseyCarter
Copy link
Contributor

From #599 (comment): we'll need to rebase this atop #599, update the value of __cpp_lib_array_constexpr, and fix libc++'s language.support\support.limits\support.limits.general\array.version.pass.cpp test.

@miscco miscco force-pushed the misc_constexpr branch 2 times, most recently from ae8900a to c1cbd20 Compare April 1, 2020 11:10
@miscco
Copy link
Contributor Author

miscco commented Apr 1, 2020

So I rebased upon @Weheineman work and now I get some erros that I really do not understand:

--
C:\agent_work\1\s\tests\std\tests\Dev11_0343056_pair_tuple_ctor_sfinae\test.cpp(150,19): error: unused variable 't3' [-Werror,-Wunused-variable]

    tuple<A*> t3(allocator_arg, al, nullptr);
              ^

C:\agent_work\1\s\tests\std\tests\Dev11_0343056_pair_tuple_ctor_sfinae\test.cpp(144,19): error: unused variable 't1' [-Werror,-Wunused-variable]

    tuple<A*> t1(allocator_arg, al);
              ^

2 errors generated.

Those variables were unused before. How does my change affect this code?

The other errors seem valid as string_view::copy was incorrectly marked as constexpr in C++17 and I will simply add a #if _HAS_CXX20

@miscco miscco force-pushed the misc_constexpr branch 2 times, most recently from fc68b5b to de741f2 Compare April 4, 2020 19:46
@miscco
Copy link
Contributor Author

miscco commented Apr 10, 2020

Rebased on master. I still have no Idea why the tests a suddenly failing.

Should we investigate or should we simply add a (void) to supress those warnings

@StephanTLavavej
Copy link
Member

I believe the product code is done - just need to review the test and extend it for overlapping ranges.

@miscco
Copy link
Contributor Author

miscco commented May 21, 2020

That isa simple and elegant solution to the mess I did with those char_traits. Thanks a lot 👍

@StephanTLavavej
Copy link
Member

Summary of test changes:

  • Unwrap comment.
  • Rename first and second tuples to avoid not-really-shadowing meow.first and meow.second.
  • In general, attempt to use unique values for test data, so corresponding values are easier to see.
  • Add asserts to verify that operations work correctly, in addition to compiling.
  • Style: Use {} to initialize pair even though it performs value-initialization.
  • Style: Use copyAssignment as the assignment source for copyAssignmentConvertible.
  • Change comment to test pair swap because both member and non-member swap are being tested. Ditto test array swap.
  • Don't test internal _Alloc_exact_args_t machinery.
  • Test more empty tuple<> operations: allocator move-construction, copy assignment, and move assignment.
  • Test more tuple operations:
    • The const Types&... constructor is rarely used, but it's mainly used for things that don't perfectly forward, like braces and 0 as a null pointer constant. Added tuple_alloc_non_forwarding_value to test this.
    • We need to move(conversionInput) and move(conversionInputPair), otherwise they'll be copied. (Note: it's okay to "reuse" moved-from objects here, since they're ultimately storing int and double etc. where moves don't actually modify them.)
    • Assignments.
  • Test empty array::fill.
  • The insert iterator tests are usually using non-const rvalues like 42, so we actually need an lvalue (or here, a const lvalue) to test the copy overload. So, change move(toBeMoved) to toBeCopied.
  • Add comprehensive tests for char_traits move/copy/assign and basic_string_view::copy. This covers the 5 character types: char, char16_t, char32_t, wchar_t, and char8_t. I chose to replicate the code instead of macroizing the literals.

@miscco
Copy link
Contributor Author

miscco commented May 22, 2020

Stephan: This is good, just some final touches.

Also Stephan: Add a zillion lines of test, fixes bad style, cleanes up everything.

You are amazing.

And terryfing

But mostly amazing

@StephanTLavavej
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephanTLavavej
Copy link
Member

One more change - I requested compiler support from MSVC (DevCom-1046483) and EDG (Microsoft-internal VSO-1129974), so I added a centralized macro that we'll be able to update in the future, with a TRANSITION comment citing the bug numbers.

@StephanTLavavej StephanTLavavej merged commit 212de15 into microsoft:master May 23, 2020
@StephanTLavavej
Copy link
Member

Thanks for building one more stage of the C++20 rocket! 🚀 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cxx20 C++20 feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

<string_view>: basic_string_view::copy() mistakenly marked constexpr before C++20 P1032R1 P1032R1 Miscellaneous constexpr

6 participants