Skip to content

Address fallout from -Wsign-conversion work on Windows#2241

Merged
gennadiycivil merged 1 commit intogoogle:masterfrom
ngie-eign:fix-wsign-conversion-fallout
Apr 26, 2019
Merged

Address fallout from -Wsign-conversion work on Windows#2241
gennadiycivil merged 1 commit intogoogle:masterfrom
ngie-eign:fix-wsign-conversion-fallout

Conversation

@ngie-eign
Copy link
Copy Markdown
Contributor

@ngie-eign ngie-eign commented Apr 26, 2019 •
edited
Loading

Some Windows users builds were broken after a0d60be. This change
addresses the lingering -Wsign-conversion issues with those platforms
by adding some missing static_cast calls as needed.

Signed-off-by: Enji Cooper yaneurabeya@gmail.com

@ngie-eign ngie-eign force-pushed the fix-wsign-conversion-fallout branch from c4fdb20 to e60570c Compare April 26, 2019 11:07
@ngie-eign ngie-eign changed the title Address fallout from -Wsign-conversion work on Cygwin Address fallout from -Wsign-conversion work on Windows Apr 26, 2019
@ngie-eign
Address fallout from -Wsign-conversion work on Windows
bd47c09 
Some Windows users builds were broken after a0d60be. This change
addresses the lingering -Wsign-conversion issues with those platforms
by adding some missing `static_cast` calls as needed.

Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>
@ngie-eign ngie-eign force-pushed the fix-wsign-conversion-fallout branch from e60570c to bd47c09 Compare April 26, 2019 11:08
@ngie-eign
Copy link
Copy Markdown
Contributor Author

ngie-eign commented Apr 26, 2019 •
edited
Loading

CC: @elcabesa, @elv-peter, @gennadiycivil. This fixes Cygwin compilation for sure and the one other Windows issue reported, but it doesn't necessarily fix all Windows issues. I will need more help chasing any remaining platform issues down, e.g., directions for setting up Visual Studio, MingW, etc, to reproduce the issues seen in the environment.

@elcabesa
Copy link
Copy Markdown

elcabesa commented Apr 26, 2019

my setup is quite easy:

  1. download mingw installer: https://sourceforge.net/projects/mingw-w64/files/latest/download
  2. install it, ( i choosed 8.1.0, x86_64, posix, seh, 0)
  3. download clang from their official page: http://releases.llvm.org/8.0.0/LLVM-8.0.0-win64.exe
    4)install it
  4. create a batch file containing the following code and run it (please customize the paths ):
echo off
set PATH=F:\clang\LLVM\bin;F:\x86_64-8.1.0-posix-seh-rt_v6-rev0\mingw64\bin;%PATH%
"C:\WINDOWS\system32\cmd.exe"

now you have a console and you can run make :)
to use clang set those variables:

set CC=clang --target=x86_64-mingw32
set CXX=clang++ --target=x86_64-mingw32

to run make you have to type mingw32-make.

if you want to use cmake you have to install it. To create the makefiles from cmake on window you can use those instructions ( i created another batch):

set CC=clang --target=x86_64-mingw32
set CXX=clang++ --target=x86_64-mingw32
mkdir build
cd build 
cmake  -DCMAKE_BUILD_TYPE=Release -G "MinGW Makefiles" .. 
mingw32-make

@elcabesa
Copy link
Copy Markdown

elcabesa commented Apr 26, 2019

your code still has some problems: one more implicit conversion:

[  1%] Building CXX object googletest-build/googletest/CMakeFiles/gtest.dir/src/gtest-all.obj
In file included from F:\vajolet22\build\googletest-src\googletest\src\gtest-all.cc:41:
F:/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
                                          ^~
1 error 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:105: googletest-build/googletest/CMakeFiles/gtest.dir/all] Error 2
mingw32-make: *** [Makefile:129: all] Error 2

@elcabesa
Copy link
Copy Markdown

elcabesa commented Apr 26, 2019

you have to change gtest.cc:1694 to:
static_cast<DWORD>(hr), // the error

@gennadiycivil gennadiycivil merged commit bd47c09 into google:master Apr 26, 2019
gennadiycivil added a commit that referenced this pull request Apr 26, 2019
@gennadiycivil
Merge pull request #2241 from ngie-eign:fix-wsign-conversion-fallout
61cdca5 
PiperOrigin-RevId: 245430295
@ngie-eign ngie-eign deleted the fix-wsign-conversion-fallout branch April 29, 2019 12:36
@elv-peter
Copy link
Copy Markdown

elv-peter commented Apr 29, 2019

@ngie-eign This was the compile error I was getting (with the Emscripten toolchain as you can see):

cd /p/e/git/ws/elv-crypto/build/emscripten/googletest/googletest-build/googletest && /p/e/git/ws/elv-toolchain/dist/darwin-10.14/share/emsdk/emscripten/1.38.12/em++  -DELV_INCLUDE_EMSCRIPTEN_ @CMakeFiles/gtest.dir/includes_CXX.rsp -DNDEBUG -O2   -Wall -Wshadow -Werror -Wconversion -DGTEST_HAS_PTHREAD=1 -fexceptions -W -Wpointer-arith -Wreturn-type -Wcast-qual -Wwrite-strings -Wswitch -Wunused-parameter -Wcast-align -Wchar-subscripts -Winline -Wredundant-decls -Wall -Wextra -pedantic -Wno-unused-parameter -std=c++11 -o CMakeFiles/gtest.dir/src/gtest-all.cc.o -c /p/e/git/ws/elv-crypto/build/emscripten/googletest/googletest-src/googletest/src/gtest-all.cc
In file included from /p/e/git/ws/elv-crypto/build/emscripten/googletest/googletest-src/googletest/src/gtest-all.cc:41:
/p/e/git/ws/elv-crypto/build/emscripten/googletest/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++);
           ~~~~~~~~ ^~~~~~
/p/e/git/ws/elv-crypto/build/emscripten/googletest/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++);
            ~~~~~~~~ ^~~~~~
2 errors generated.

This fixes it:

left = towlower(static_cast<wint_t>(*lhs++));
right = towlower(static_cast<wint_t>(*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.

5 participants

@ngie-eign @elcabesa @elv-peter @googlebot @gennadiycivil