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

<chrono>: Add comments for compiler bug workarounds #4920

Merged
merged 5 commits into from
Aug 28, 2024

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Aug 27, 2024

The internal function template tzdb_list::_Emplace_front (initially added in #1671) only exists for testing. I found that it's possibly to completely hack in the test codes, so this PR removes it.

After Clang 16 got WG21-P0960R3 implemented, it should be no longer necessary to construct a tzdb before calling emplace_front. It seems more efficient to directly construct the tzdb element within emplace_front. But there's some bug in EDG so perhaps it's better to give up the change.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner August 27, 2024 16:26
@CaseyCarter CaseyCarter added the enhancement Something can be improved label Aug 27, 2024
@CaseyCarter
Copy link
Member

I guess that was also a workaround for EDG. We'll need to reduce a repro to send to the Edge team and conditionally (or unconditionally) restore the workaround with a transition comment.

@CaseyCarter
Copy link
Member

CaseyCarter commented Aug 27, 2024

We'll need to reduce a repro to send to the Edge team

Reduced to:

struct S {
    int x, y;
};

void f(void* ptr) {
    S(13, 42); // Ok
    new S(13, 42); // error: no instance of constructor "S::S" matches
}

Which I filed as VSO-2228186 and added to #1621.

@frederick-vs-ja frederick-vs-ja changed the title <chrono>: Simplify tzdb_list a bit around emplace_front <chrono>: Remove testing-only internal function template tzdb_list::_Emplace_front Aug 27, 2024
@CaseyCarter CaseyCarter added test Related to test code and removed enhancement Something can be improved labels Aug 27, 2024
@StephanTLavavej StephanTLavavej self-assigned this Aug 27, 2024
@StephanTLavavej StephanTLavavej removed their assignment Aug 27, 2024
@StephanTLavavej StephanTLavavej self-assigned this Aug 27, 2024
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej removed their assignment Aug 27, 2024
@StephanTLavavej StephanTLavavej added the decision needed We need to choose something before working on this label Aug 27, 2024
@StephanTLavavej
Copy link
Member

/clr hates this technique, reduced and reported as VSO-2231268 "/clr /std:c++20 rejects sneaky technique to access private data members".

Instead of guarding this test coverage for /clr, I'm now inclined to abandon the private data member hack, and merge only the TRANSITION comments for EDG. I think this is adding too much test complexity and the cost for product code is minimal.

@StephanTLavavej StephanTLavavej removed the decision needed We need to choose something before working on this label Aug 27, 2024
@StephanTLavavej StephanTLavavej changed the title <chrono>: Remove testing-only internal function template tzdb_list::_Emplace_front <chrono>: Add comments for compiler bug workarounds Aug 27, 2024
@StephanTLavavej StephanTLavavej added documentation Related to documentation or comments and removed test Related to test code labels Aug 27, 2024
@StephanTLavavej StephanTLavavej merged commit 1acbeae into microsoft:main Aug 28, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for helping us find and report so many compiler bugs! 🐞 ⏱️ 😹

@frederick-vs-ja frederick-vs-ja deleted the emplace-front-tzdb branch August 28, 2024 05:52
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
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants