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

Categorize some failing libcxx tests #5231

Merged

Conversation

muellerj2
Copy link
Contributor

... which are mostly related to <regex>.

@muellerj2 muellerj2 requested a review from a team as a code owner January 12, 2025 17:10
tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
Comment on lines +568 to +573
# We disagree on the syntax flags set by the default basic_regex constructor: we set 0, libc++ sets ECMAScript.
# Relates to LWG-3604.
std/re/re.regex/re.regex.construct/default.pass.cpp FAIL
std/re/re.regex/re.regex.nonmemb/re.regex.nmswap/swap.pass.cpp FAIL
std/re/re.regex/re.regex.swap/swap.pass.cpp FAIL

Copy link
Contributor Author

@muellerj2 muellerj2 Jan 12, 2025

Choose a reason for hiding this comment

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

I'm not really sure where to put this one. The underlying issue is that [re.regex.construct]/1 does not specify the value returned by flags() as a postcondition on the default constructor, so MSVC STL returns 0 (which could be considered more in line with the postconditions on all the other constructors that require that flags() must return the flags passed to the constructor unchanged) while libc++ returns regex_constants::ECMAScript (which is probably informed by [re.synopt]/1's statement that the default grammar is ECMAScript if no other grammar is requested).

LWG-3604 is related, but discusses the postcondition on the flags imposed by all the other constructors and argues that they are too strict. Here, the problem is kind of the opposite: The standard doesn't state a postcondition on the flags for the default constructor at all.

Copy link
Member

Choose a reason for hiding this comment

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

LWG-3604 does discuss this problem at the end:

Also, the constructors say "Postconditions: flags() returns f." This prevents an implementation from storing f|ECMAScript in flags() if no grammar element is present in f. This seems like an unnecessary restriction, and forces implementations to do extra work to check if the ECMAScript grammar is in use. Arguably, it would even be better to require implementations to set ECMAScript in flags() if no grammar element was set in the flags passed to the constructor. This problem was introduced by LWG 2330(i).

I agree that there's no obvious place for this one - "LIKELY BOGUS TESTS" seems as good as anything, since if the Standard is vague on what happens, the test shouldn't be expecting something specific. Good enough for me, and a vast improvement over the status quo of "not analyzed".

@StephanTLavavej StephanTLavavej added the test Related to test code label Jan 12, 2025
tests/libcxx/expected_results.txt Outdated Show resolved Hide resolved
Comment on lines +568 to +573
# We disagree on the syntax flags set by the default basic_regex constructor: we set 0, libc++ sets ECMAScript.
# Relates to LWG-3604.
std/re/re.regex/re.regex.construct/default.pass.cpp FAIL
std/re/re.regex/re.regex.nonmemb/re.regex.nmswap/swap.pass.cpp FAIL
std/re/re.regex/re.regex.swap/swap.pass.cpp FAIL

Copy link
Member

Choose a reason for hiding this comment

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

LWG-3604 does discuss this problem at the end:

Also, the constructors say "Postconditions: flags() returns f." This prevents an implementation from storing f|ECMAScript in flags() if no grammar element is present in f. This seems like an unnecessary restriction, and forces implementations to do extra work to check if the ECMAScript grammar is in use. Arguably, it would even be better to require implementations to set ECMAScript in flags() if no grammar element was set in the flags passed to the constructor. This problem was introduced by LWG 2330(i).

I agree that there's no obvious place for this one - "LIKELY BOGUS TESTS" seems as good as anything, since if the Standard is vague on what happens, the test shouldn't be expecting something specific. Good enough for me, and a vast improvement over the status quo of "not analyzed".

@StephanTLavavej
Copy link
Member

Thanks, I love seeing progress on categorizing test failures!! 😻

@StephanTLavavej StephanTLavavej self-assigned this Jan 13, 2025
@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 91a8d4f into microsoft:main Jan 14, 2025
39 checks passed
@StephanTLavavej
Copy link
Member

📜 📄 📃

@muellerj2 muellerj2 deleted the categorize-libcxx-test-failures branch January 14, 2025 20:24
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
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants