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

update stl/inc/xtr1common: _Is_any_of_v #3933

Merged
merged 2 commits into from
Aug 11, 2023

Conversation

GoodenoughPhysicsLab
Copy link
Contributor

This is a very simple modification. The original _Is_any_of_v produces a lot of bloated unrolling. Using fold-expression is a better alternative. Because the change was so simple, I didn't issue it, and the tests were fine.

@GoodenoughPhysicsLab GoodenoughPhysicsLab requested a review from a team as a code owner August 7, 2023 14:22
Copy link
Contributor

@fsb4000 fsb4000 left a comment

Choose a reason for hiding this comment

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

We use different style for comments:

STL/stl/inc/list

Lines 984 to 988 in ed8150e

#if _HAS_CXX17
return _Result;
#else // ^^^ _HAS_CXX17 / !_HAS_CXX17 vvv
(void) _Result;
#endif // _HAS_CXX17

but I don't think it's worth to rerun tests to fix the comments.

Yeah, fold-expression could have better throughput.

@GoodenoughPhysicsLab
Copy link
Contributor Author

Sorry, I was so reckless.

@GoodenoughPhysicsLab
Copy link
Contributor Author

This was my first attempt to submit a pr to a project like this, and there were a lot of things I didn't do well

@fsb4000
Copy link
Contributor

fsb4000 commented Aug 7, 2023

The CI will be restarted later.

I have no idea what's wrong: "Received request to deprovision: The request was cancelled by the remote provider."

But your code is fine.

No changes needed.

@CaseyCarter
Copy link
Member

I have no idea what's wrong: "Received request to deprovision: The request was cancelled by the remote provider."

We use so-called "spot" VMs in Azure to minimize CI costs. They cost less, but are subject to being pulled out from underneath us without warning and reallocated to full paying customers.

@StephanTLavavej StephanTLavavej added the throughput Must compile faster label Aug 7, 2023
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 7, 2023

The MSVC compiler actually internally optimizes both disjunction_v and is_same_of_v, so I expect that this will make no difference for MSVC. It might help Clang and shouldn't hurt.

@StephanTLavavej StephanTLavavej self-assigned this Aug 7, 2023
@StephanTLavavej
Copy link
Member

We use _Is_any_of_v commonly enough that this seems like a reasonable, targeted change to make. I'd be skeptical of further changes to conditionally replace usage of conjunction_v/disjunction_v with fold expressions.

@StephanTLavavej StephanTLavavej removed their assignment Aug 7, 2023
@StephanTLavavej StephanTLavavej changed the title update stl/inc/xtrlcommon: _Is_any_of_v update stl/inc/xtr1common: _Is_any_of_v Aug 8, 2023
@StephanTLavavej StephanTLavavej self-assigned this Aug 10, 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 094e533 into microsoft:main Aug 11, 2023
35 checks passed
@StephanTLavavej
Copy link
Member

Thanks for improving compiler throughput here, and congratulations on your first microsoft/STL commit! 🎉 😻 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
throughput Must compile faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants