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

Uncomment resolutions of applied LWG issues #3461

Merged
merged 2 commits into from
Feb 14, 2023

Conversation

CaseyCarter
Copy link
Contributor

@CaseyCarter CaseyCarter commented Feb 13, 2023

Covers:

I've verified that we completely implement the resolutions of these issues.

Drive-by: remove extraneous () in the constraints on basic_string_view's range constructor.

@CaseyCarter CaseyCarter added the documentation Related to documentation or comments label Feb 13, 2023
@CaseyCarter CaseyCarter requested a review from a team as a code owner February 13, 2023 02:50
Covers:
* LWG-3085
* LWG-3664
* LWG-3723
* LWG-3742
* LWG-3807
* LWG-3857
* LWG-3867

I've verified that we completely implement the resolutions of these issues.

Drive-by: remove extraneous `()` in the constraints on `basic_string_view`'s range constructor.
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

This is missing changes in:

  • tests/libcxx/expected_results.txt:623
  • tests/libcxx/skipped_tests.txt:623
  • tests/std/tests/P0220R1_string_view/test.cpp:376
diff --git a/tests/libcxx/expected_results.txt b/tests/libcxx/expected_results.txt
index dcb235da..bf43ff99 100644
--- a/tests/libcxx/expected_results.txt
+++ b/tests/libcxx/expected_results.txt
@@ -620,7 +620,7 @@ std/language.support/support.limits/support.limits.general/format.version.compil
 # libc++ doesn't yet implement LWG-3670
 std/ranges/range.factories/range.iota.view/iterator/member_typedefs.compile.pass.cpp FAIL
 
-# libc++ doesn't speculatively implement LWG-3857
+# libc++ doesn't yet implement LWG-3857
 std/strings/string.view/string.view.cons/from_range.pass.cpp FAIL
 std/strings/string.view/string.view.cons/from_string1.compile.fail.cpp FAIL
 std/strings/string.view/string.view.cons/from_string2.compile.fail.cpp FAIL
diff --git a/tests/libcxx/skipped_tests.txt b/tests/libcxx/skipped_tests.txt
index 6f494826..95e3cc63 100644
--- a/tests/libcxx/skipped_tests.txt
+++ b/tests/libcxx/skipped_tests.txt
@@ -620,7 +620,7 @@ language.support\support.limits\support.limits.general\format.version.compile.pa
 # libc++ doesn't yet implement LWG-3670
 ranges\range.factories\range.iota.view\iterator\member_typedefs.compile.pass.cpp
 
-# libc++ doesn't speculatively implement LWG-3857
+# libc++ doesn't yet implement LWG-3857
 strings\string.view\string.view.cons\from_range.pass.cpp
 strings\string.view\string.view.cons\from_string1.compile.fail.cpp
 strings\string.view\string.view.cons\from_string2.compile.fail.cpp
diff --git a/tests/std/tests/P0220R1_string_view/test.cpp b/tests/std/tests/P0220R1_string_view/test.cpp
index 78324c1c..0484c73d 100644
--- a/tests/std/tests/P0220R1_string_view/test.cpp
+++ b/tests/std/tests/P0220R1_string_view/test.cpp
@@ -373,7 +373,6 @@ constexpr bool test_case_range_constructor() {
     static_assert(!is_convertible_v<string_view, basic_string_view<char, constexpr_char_traits>>);
     static_assert(!is_convertible_v<basic_string_view<char, constexpr_char_traits>, string>);
 
-    // LWG-3857 basic_string_view should allow explicit conversion when only traits vary
     static_assert(is_constructible_v<string_view, basic_string<char, constexpr_char_traits>>);
     static_assert(is_constructible_v<basic_string_view<char, constexpr_char_traits>, string_view>);

@CaseyCarter
Copy link
Contributor Author

CaseyCarter commented Feb 13, 2023

This is missing changes in:

  • tests/libcxx/expected_results.txt:623
  • tests/libcxx/skipped_tests.txt:623

These are fixed in #3464. (I did some more extensive edits to these files, and wanted to avoid merge conflicts, so I left them alone in this PR.)

  • tests/std/tests/P0220R1_string_view/test.cpp:376

Yeow - fixed. (I audited for other missed occurrences of all the pertinent LWG issues, and only found the mentions of LWG-3857 in the skip files you mention above.)

@StephanTLavavej
Copy link
Member

I am neutral on the P0220R1_string_view change - while product code doesn't cite resolved LWG issues, test code often does (similar to how we cite bugs, even though ultimately it's the Standard telling us what to do), and this one just mentioned the number, instead of saying "per the proposed resolution". That said, this one is simple so the LWG issue doesn't provide significant useful context.

@StephanTLavavej StephanTLavavej self-assigned this Feb 13, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 3b79276 into microsoft:main Feb 14, 2023
@StephanTLavavej
Copy link
Member

// Thanks for removing these comments! 😻🎉🚀

@CaseyCarter CaseyCarter deleted the uncomment branch February 14, 2023 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Related to documentation or comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants