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 now treated as error #2107

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

ahesham-arm
Copy link
Contributor

Following the re-enablement of narrowing warnings, this fixes a compilation error when running the ubuntu 22.04 arm GitHub action.

Following the re-enablement of narrowing warnings, this fixes a
compilation error when running the `ubuntu 22.04 arm` GitHub action.

Signed-off-by: Ahmed Hesham <[email protected]>
Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

Non-controversial, merging immediately.

(Is this something we could/should have caught in CI?)

@bashbaug bashbaug merged commit 1527c4e into KhronosGroup:main Oct 8, 2024
7 checks passed
@ahesham-arm ahesham-arm deleted the compilation_error branch October 9, 2024 07:21
@ahesham-arm
Copy link
Contributor Author

Non-controversial, merging immediately.

(Is this something we could/should have caught in CI?)

@bashbaug Thank you for the quick review and merge. I think the problem was that the pull request had passed all the CI checks when it was merged, the compilation error was a from a file that was added after that point. I don't think it would have been caught by the CI except if we had asked to rerun the checks before merging (not unreasonable but will not likely scale well with the volume of pull requests we have open).

@coldav
Copy link

coldav commented Oct 9, 2024

This change breaks 32 bit builds (built on x86_64 linux with -m32):

/extensions/cl_khr_semaphore/test_semaphores_negative_wait_signal.cpp:91:58: error: narrowing conversion of '(maxComputeUnits / 2)' from 'cl_uint' {aka 'unsigned int'} to 'cl_device_partition_property' {aka 'int'} [-Werror=narrowing] 91 | CL_DEVICE_PARTITION_EQUALLY, maxComputeUnits / 2, 0

Do you need an issue created or do you want to just fix the new error?

As a side-note we have had other 32 bit fails for similar reasons - should we add 32 bit x86_64 build? (+test?)

@ahesham-arm
Copy link
Contributor Author

This change breaks 32 bit builds (built on x86_64 linux with -m32):

/extensions/cl_khr_semaphore/test_semaphores_negative_wait_signal.cpp:91:58: error: narrowing conversion of '(maxComputeUnits / 2)' from 'cl_uint' {aka 'unsigned int'} to 'cl_device_partition_property' {aka 'int'} [-Werror=narrowing] 91 | CL_DEVICE_PARTITION_EQUALLY, maxComputeUnits / 2, 0

Do you need an issue created or do you want to just fix the new error?

As a side-note we have had other 32 bit fails for similar reasons - should we add 32 bit x86_64 build? (+test?)

Are you sure this change breaks your build? The error you pasted seems to reference the original code before this fix is applied.

@coldav
Copy link

coldav commented Oct 9, 2024

Ah yes, I think you are right, we saw a fail last night and i checked the latest commits. My apologies.

svenvh pushed a commit that referenced this pull request Oct 16, 2024
…rms (#2117)

Looks like #2063 has a
"narrowing" warning that is now treated as an error and is hence causing
CI builds to fail.

Applies the same fix as
#2107 to fix this
warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants