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

Enable __cpp_lib_concepts for EDG, part 1 #4296

Merged
merged 39 commits into from
Feb 1, 2024

Conversation

cpplearner
Copy link
Contributor

@cpplearner cpplearner commented Jan 4, 2024

This PR makes __cpp_lib_concepts defined in C++20 mode for all supported compilers, after adding workarounds for various EDG concepts bugs. I have opened two follow-up PRs for non-essential and largely mechanical clean-ups.

…alizations that differ only in the requires-clause
…_copy_assignable_type> to be trivially copy-assignable
…r the private members can be accessed in the current context
@cpplearner cpplearner requested a review from a team as a code owner January 4, 2024 11:33
@CaseyCarter CaseyCarter self-requested a review January 26, 2024 19:58
@CaseyCarter

This comment was marked as resolved.

@CaseyCarter CaseyCarter removed their assignment Jan 31, 2024
@StephanTLavavej
Copy link
Member

They were TrivialityScenario4 "Only destruction and move construction/assignment are trivial" and TrivialityScenario6 "All operations are trivial", revealing @CaseyCarter's deep wisdom in #4271 (comment) of testing these scenarios. 😻

@StephanTLavavej StephanTLavavej self-assigned this Jan 31, 2024
@StephanTLavavej StephanTLavavej removed their assignment Feb 1, 2024
@StephanTLavavej StephanTLavavej self-assigned this Feb 1, 2024
@StephanTLavavej
Copy link
Member

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

DevCom-10265237 "Parameter pack size mismatch doesn't make the require-expression false (regression)" is an MSVC bug.

This preprocessor logic is guarding an additional constraint that MSVC needs, so I'm updating the `#endif` comment to "^^^ workaround ^^^".

Previously, I believe this code was mentally saying, "we're in concepts code, so we only need to distinguish Clang from MSVC". This is no longer the case. Now we need `!defined(__clang__) && !defined(__EDG__)` to prevent EDG from using a workaround that it doesn't need.
DevCom-1691516 "std::vector::emplace_back does not work for a class with default initializer" is an MSVC bug.

Curiously, this preprocessor logic was checking only `#ifdef __EDG__` to distinguish it from MSVC. (I think it might have been written before Clang supported construct_at()?)

Instead, we need to check `#if defined(__clang__) || defined(__EDG__)` and send them to the "no workaround" case. This prevents Clang from using a workaround that it doesn't need.
@StephanTLavavej
Copy link
Member

I've pushed additional commits:

  • Attempt to avoid timeouts in P0896R4_ranges_alg_find_end.
    • Encountered in the internal test harness, probably because the "checked" compiler consumes more memory.
  • Drop "assert bug so we'll notice when it's fixed".
    • I know this had worthy intentions, but it's a huge headache when either MSVC or EDG devs are trying to test their fixed compilers.
  • Properly detect MSVC, part 1.
    • DevCom-10265237 "Parameter pack size mismatch doesn't make the require-expression false (regression)" is an MSVC bug.
    • This preprocessor logic is guarding an additional constraint that MSVC needs, so I'm updating the #endif comment to ^^^ workaround ^^^.
    • Previously, I believe this code was mentally saying, "we're in concepts code, so we only need to distinguish Clang from MSVC". This is no longer the case. Now we need !defined(__clang__) && !defined(__EDG__) to prevent EDG from using a workaround that it doesn't need.
  • Properly detect MSVC, part 2.
    • DevCom-1691516 "std::vector::emplace_back does not work for a class with default initializer" is an MSVC bug.
    • Curiously, this preprocessor logic was checking only #ifdef __EDG__ to distinguish it from MSVC. (I think it might have been written before Clang supported construct_at()?)
    • Instead, we need to check #if defined(__clang__) || defined(__EDG__) and send them to the "no workaround" case. This prevents Clang from using a workaround that it doesn't need.

I have comment cleanups for a followup PR but I didn't want to further randomize this one when it's so close to landing.

@StephanTLavavej StephanTLavavej merged commit 446444e into microsoft:main Feb 1, 2024
35 checks passed
@StephanTLavavej
Copy link
Member

😻 🎉 🐈 🚀 🍰

Thank you @cpplearner and @CaseyCarter for the massive amount of work needed to get to this long-awaited day!

@cpplearner cpplearner deleted the edg-concepts-1 branch February 2, 2024 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants