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.

Sign up for GitHub

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

Jump to bottom

Fix -Wsign-conversion issues #2170

Closed
wants to merge 1 commit into from
Closed

Fix -Wsign-conversion issues #2170

wants to merge 1 commit into from

Conversation

ngie-eign
Copy link
Contributor

@ngie-eign ngie-eign commented Mar 8, 2019 •
edited

This PR takes a different approach by tackling individuals issues without changing internal API construction, as I tried in #2165.

This makes googletest compile cleanly with clang++ -Wconversion, fixing #2146.

@EricWF
Copy link

EricWF commented Mar 9, 2019

This patch looks a lot better. Thank you.

@ngie-eign
Copy link
Contributor Author

Thanks for the feedback @EricWF! I’ll force-push my newer revision soon which touches more areas affected by this warning.

@ngie-eign ngie-eign force-pushed the issue-2146-ver2 branch 2 times, most recently from d453575 to b81db4e Compare March 21, 2019 09:01
@ngie-eign ngie-eign changed the title Proof-of-concept: fix -Wsign-conversion issues (version 2) Fix -Wsign-conversion issues Mar 21, 2019
@ngie-eign
Copy link
Contributor Author

I've addressed the remaining issues via casting, etc. Please feel free to review!

@ngie-eign
Copy link
Contributor Author

@gennadiycivil: ping?

@ngie-eign
Copy link
Contributor Author

I see there are issues I need to resolve before this can be merged.

@ngie-eign ngie-eign force-pushed the issue-2146-ver2 branch 10 times, most recently from 92c1238 to 55f2d3a Compare April 3, 2019 07:38
@ngie-eign
Copy link
Contributor Author

@EricWF, @gennadiycivil: I fixed the issues, so CI once again passes on this branch (and I fixed some issues with incoming code as well in the past few days). Please re-review this PR. Thank you!

@ngie-eign
Copy link
Contributor Author

And I broke it by accident with a force push.. I will unbreak it again soon.

@ngie-eign
Copy link
Contributor Author

And I broke it by accident with a force push.. I will unbreak it again soon.

Ok. Force pushed the right set of changes.

@ngie-eign
Copy link
Contributor Author

@EricWF, @gennadiycivil: ready for review!

@ngie-eign
Copy link
Contributor Author

Ping?

@ngie-eign
Copy link
Contributor Author

@EricWF, @gennadiycivil: ping?

@ngie-eign
Copy link
Contributor Author

@gennadiycivil: ping?

@gennadiycivil
Copy link
Contributor

Looking, sorry for the delay

@ngie-eign ngie-eign force-pushed the issue-2146-ver2 branch 3 times, most recently from b262124 to 243f815 Compare April 17, 2019 16:34
@ngie-eign
clang: fix -Wsign-conversion errors
9358a33 
Cast some values as their unsigned equivalents or `size_t` to match the
parameter type used for the template object under test. Also, provide
UInt32 equivalent delegate methods for some callers (with
int-equivalents for backwards compatibility).

This closes google#2146.

Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>
@ngie-eign
Copy link
Contributor Author

Looking, sorry for the delay

I'm trying again after rebasing. It seems that other changes have snuck in, in the past week, which are triggering failures on Linux >_>.

@ngie-eign
Copy link
Contributor Author

@gennadiycivil: ok. CI is green again with the latest set of pushed changes.

gennadiycivil added a commit that referenced this pull request Apr 18, 2019
@gennadiycivil
Merge pull request #2170 from ngie-eign:issue-2146-ver2
a0d60be 
PiperOrigin-RevId: 244069956
@gennadiycivil
Copy link
Contributor

We had an internal review and made small-ish changes, mostly formatting and trivial. I imported this PR before your latest commit so your latest changes are not here. The most expedient would be to make another PR to pick up the changes that could have been missed.
Also please review the formatting changes we made.

Thank you

@elcabesa
Copy link

just try to compile google test a0d60be with clang 7.0.1
this is the error coming out from cmake:

[ 1%] Building CXX object googletest-build/googletest/CMakeFiles/gtest.dir/src/gtest-all.obj
In file included from C:\Users\elcab\Downloads\workspace\vajolet22\build\googletest-src\googletest\src\gtest-all.cc:41:
C:/Users/elcab/Downloads/workspace/vajolet22/build/googletest-src/googletest\src/gtest.cc:1694:43: error: implicit
conversion changes signedness: 'long' to 'DWORD' (aka 'unsigned long') [-Werror,-Wsign-conversion]
hr, // the error
^~
In file included from C:\Users\elcab\Downloads\workspace\vajolet22\build\googletest-src\googletest\src\gtest-all.cc:45:
C:/Users/elcab/Downloads/workspace/vajolet22/build/googletest-src/googletest\src/gtest-port.cc:282:11: error: implicit
conversion changes signedness: 'int' to 'DWORD' (aka 'unsigned long') [-Werror,-Wsign-conversion]
::Sleep(n);
~~ ^
In file included from C:\Users\elcab\Downloads\workspace\vajolet22\build\googletest-src\googletest\src\gtest-all.cc:46:
C:/Users/elcab/Downloads/workspace/vajolet22/build/googletest-src/googletest\src/gtest-printers.cc:179:28: error:
implicit conversion changes signedness: 'signed char' to 'wchar_t' [-Werror,-Wsign-conversion]
if (IsPrintableAscii(c)) {
~~~~~~~~~~~~~~~~ ^
C:/Users/elcab/Downloads/workspace/vajolet22/build/googletest-src/googletest\src/gtest-printers.cc:223:29: note: in
instantiation of function template specialization 'testing::internal::PrintAsCharLiteralTo<unsigned char, signed
char>' requested here
const CharFormat format = PrintAsCharLiteralTo(c, os);
^
C:/Users/elcab/Downloads/workspace/vajolet22/build/googletest-src/googletest\src/gtest-printers.cc:248:3: note: in
instantiation of function template specialization 'testing::internal::PrintCharAndCodeTo<unsigned char, signed
char>' requested here
PrintCharAndCodeTo(c, os);
^
3 errors generated.
mingw32-make[2]: *** [googletest-build\googletest\CMakeFiles\gtest.dir\build.make:63: googletest-build/googletest/CMakeFiles/gtest.dir/src/gtest-all.obj] Error 1
mingw32-make[1]: *** [CMakeFiles\Makefile2:110: googletest-build/googletest/CMakeFiles/gtest.dir/all] Error 2
mingw32-make: *** [Makefile:129: all] Error 2

@ngie-eign
Copy link
Contributor Author

ngie-eign commented Apr 18, 2019 via email

@ngie-eign ngie-eign deleted the issue-2146-ver2 branch April 22, 2019 20:44
@elv-peter
Copy link

Broke my Emscripten build I think:

In file included from .../googletest-src/googletest/src/gtest-all.cc:41:
.../googletest-src/googletest/src/gtest.cc:1952:21: error: implicit conversion
      changes signedness: 'const wchar_t' to 'wint_t' (aka 'unsigned int') [-Werror,-Wsign-conversion]
    left = towlower(*lhs++);
           ~~~~~~~~ ^~~~~~
.../googletest-src/googletest/src/gtest.cc:1953:22: error: implicit conversion
      changes signedness: 'const wchar_t' to 'wint_t' (aka 'unsigned int') [-Werror,-Wsign-conversion]
    right = towlower(*rhs++);
            ~~~~~~~~ ^~~~~~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ngie-eign @EricWF @gennadiycivil @elcabesa @elv-peter @googlebot