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

Fix warning and compilation errors #1849

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

Xewar313
Copy link
Contributor

@Xewar313 Xewar313 commented Sep 17, 2024

When oneDPL is compiled without spirv, code in parallel_backend_sycl_radix_sort_one_wg.h causes unused variable error, so removing this variable should help.

Presence of function definitions in header files sometimes causes multiple definitions error, changing them to inline should solve the issue

Copy link
Contributor

@lslusarczyk lslusarczyk left a comment

Choose a reason for hiding this comment

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

LGTM, @Xewar313 thank you for fixing bug that blocks distribute-ranges compilation

@lslusarczyk lslusarczyk merged commit c4c7c27 into uxlfoundation:main Sep 17, 2024
23 checks passed
Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev left a comment

Choose a reason for hiding this comment

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

Hi @Xewar313. Thank you for this important fix. The changes look good enough to me.

Meanwhile, I've prioritized #1229 to detect ODR violations earlier, and added #1850 for detecting warnings like the fixed one.

@dmitriy-sobolev
Copy link
Contributor

I've added #1851 which reverts unnecessary refactoring changes. Besides that, it would have been great to have separate PRs for each problem solved, but it is not critical, especially after merging.

@dmitriy-sobolev
Copy link
Contributor

@Xewar313, do not hesitate to create a PR with the description of your contribution in https://github.com/oneapi-src/oneDPL/blob/main/CREDITS.txt if you wish it to be mentioned there. It certainly deserves it.

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

Successfully merging this pull request may close these issues.

3 participants