-
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
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
|
Should not we then check the clang's version and enable concepts only if the version is not 17 or not 19? We have |
I agree, we can enable back concepts on the clang versions not affected by the issue described above. I still propose to keep concepts on clang totally disabled for this release and create a separate issue for enabling it to make sure other fixes introduced in this patch are landed. I still feel that we need to test the potential fix better on different versions of clang on both Linux and MacOS before enabling concepts. |
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.
Okay, let's have it disabled for Clang in this release.
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
Thanks for this. |
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