-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix C++20 macro in parallel_for_each + fix concepts macro definition #1611
base: master
Are you sure you want to change the base?
Conversation
@kboyarinov @dnmokhov @sarathnandu Could this PR please be prioritized for review? The bug is significantly impacting oneDPL's pass rate here as it shows up in our oneTBB backend, and it needs to be fixed for our upcoming milestone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but some tests on macOS fail. Is it because of the compiler not supporting concept specifics we use?
….com/oneapi-src/oneTBB into dev/kboyarinov/for_each_concepts_fix
@aleksei-fedotov @pavelkumbrasev @isaevil @sarathnandu @dnmokhov |
I agree, since this patch is affecting oneDPL's pass rate and they are waiting for this patch to be merged. We can investigate the MacOS build issue separately. |
I think this is a bug in Clang (not only AppleClang): The bug is fixed in version LLVM 18.1.0. AppleClang from Xcode 16.2 based on LLVM 17. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The icpx compiler is also failing. I have tested on Intel(R) oneAPI DPC++/C++ Compiler 2023.2.0 (2023.2.0.20230721)
I believe you need to disable it for all clang not just clang && APPLE.
I agree that we should just disable these concepts with any Clang for now. My results of compiling
|
Description
Add a comprehensive description of proposed changes
Fix two issues:
Fixes #1552
Type of change
Choose one or multiple, leave empty if none of the other choices apply
Add a respective label(s) to PR if you have permissions
Tests
Documentation
Breaks backward compatibility
Notify the following users
List users with
@
to send notificationsOther information