Skip to content

Conversation

@ngie-eign
Copy link
Contributor

@ngie-eign ngie-eign commented Apr 26, 2019

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 [email protected]

@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
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 <[email protected]>
@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
Contributor Author

ngie-eign commented Apr 26, 2019

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

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

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

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
@ngie-eign ngie-eign deleted the fix-wsign-conversion-fallout branch April 29, 2019 12:36
@elv-peter
Copy link

@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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants