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 a warning which is treated as en error in Clang #14

Merged
merged 1 commit into from
Feb 23, 2021

Conversation

fengga
Copy link
Contributor

@fengga fengga commented Feb 22, 2021

I'm a dev from Microsoft and our product dependents on pevent. Recent changes will bring a warning and this warning in Clang is treated as an error:

uint32_t waitUnit = (WAIT_INFINITE - 1);

\deps\pevents\src\pevents.cpp(520,48): error : implicit conversion from 'unsigned long long' to 'uint32_t' (aka 'unsigned int') changes value from 18446744073709551614 to 4294967294 [-Werror,-Wconstant-conversion] [C:\MSAL3\microsoft-authentication-library-for-cpp_builds\vs-16-2019-win64-llvm-cxx17\source\msalsdk.vcxproj]

@mqudsi
Copy link
Member

mqudsi commented Feb 22, 2021

Hello @fengga and thanks for the PR.

I test against GCC9 and LLVM9 and didn't run into the conversion warning you're seeing, but then again, unsigned long long is 64-bits for me. I'm guessing you're compiling a 32-bit version of the library? I should probably add that to the CI.

@fengga
Copy link
Contributor Author

fengga commented Feb 22, 2021

Hello @fengga and thanks for the PR.

I test against GCC9 and LLVM9 and didn't run into the conversion warning you're seeing, but then again, unsigned long long is 64-bits for me. I'm guessing you're compiling a 32-bit version of the library? I should probably add that to the CI.

Hi @mqudsi, thanks for your quick response. I'm building a x64 lib using Clang 10.0.0 inside visual studio on Windows, and this issue is treated as an error (not a warning).

@mqudsi mqudsi merged commit edb32b2 into neosmart:master Feb 23, 2021
@mqudsi
Copy link
Member

mqudsi commented Feb 23, 2021

Interesting - thanks for the info.

I've merged the PR with a minor change to downcast before subtracting one in case any compilers still reason about truncation from 64-bits to 32-bits differently if it is all 0xFF. Thanks for filing!

@mqudsi
Copy link
Member

mqudsi commented Feb 23, 2021

@fengga I fixed one more downcast site and added a Windows target to the CI (I verified that CL reported the error as well); hopefully that'll catch it in the future.

Do you mind sharing what your team at MSFT is using this library for? I was previously only officially aware of its usage in Visual Studio Code.

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.

2 participants