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

Add -Werror to the list of CXXFLAGS #40

Closed
wants to merge 1 commit into from
Closed

Conversation

jfly
Copy link
Member

@jfly jfly commented Nov 6, 2023

How strict do we want to be in this repo?

Before

$ make build
...
g++ -I./src/cpp/cityhash/src -c -O3 -Wextra -Wall -pedantic -std=c++17 -g -Wsign-compare -DTWSEARCH_VERSION=v0.6.4-50-gdc1ec4e -DUSE_PTHREADS -DHAVE_FFSLL src/cpp/coset.cpp -o build/cpp/coset.o
src/cpp/coset.cpp: In function ‘int cosetcallback(setval, const std::vector<int>&, int, int)’:
src/cpp/coset.cpp:111:11: warning: array subscript 1 is outside array bounds of ‘setval [1]’ [-Warray-bounds]
  111 |     domove(pd, pos, moves[i], *(&pos + 1));
      |     ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/cpp/coset.cpp:98:26: note: at offset 8 into object ‘pos’ of size 8
   98 | int cosetcallback(setval pos, const vector<int> &moves, int d, int id) {
      |                   ~~~~~~~^~~
...
g++ -O3 -Wextra -Wall -pedantic -std=c++17 -g -Wsign-compare -o build/bin/twsearch build/cpp/antipode.o build/cpp/calcsymm.o build/cpp/canon.o build/cpp/cmdlineops.o build/cpp/filtermoves.o build/cpp/findalgo.o build/cpp/generatingset.o build/cpp/god.o build/cpp/index.o build/cpp/parsemoves.o build/cpp/prunetable.o build/cpp/puzdef.o build/cpp/readksolve.o build/cpp/solve.o build/cpp/test.o build/cpp/threads.o build/cpp/twsearch.o build/cpp/util.o build/cpp/workchunks.o build/cpp/rotations.o build/cpp/orderedgs.o build/cpp/ffi/ffi_api.o build/cpp/ffi/wasm_api.o build/cpp/city.o build/cpp/coset.o build/cpp/descsets.o build/cpp/ordertree.o build/cpp/unrotate.o build/cpp/shorten.o -lpthread

After

$ make build
...
src/cpp/coset.cpp: In function ‘int cosetcallback(setval, const std::vector<int>&, int, int)’:
src/cpp/coset.cpp:111:11: error: array subscript 1 is outside array bounds of ‘setval [1]’ [-Werror=array-bounds]
  111 |     domove(pd, pos, moves[i], *(&pos + 1));
      |     ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/cpp/coset.cpp:98:26: note: at offset 8 into object ‘pos’ of size 8
   98 | int cosetcallback(setval pos, const vector<int> &moves, int d, int id) {
      |                   ~~~~~~~^~~
cc1plus: all warnings being treated as errors
make: *** [Makefile:110: build/cpp/coset.o] Error 1

How strict do we want to be in this repo?

Before
======

    $ make build
    ...
    g++ -I./src/cpp/cityhash/src -c -O3 -Wextra -Wall -pedantic -std=c++17 -g -Wsign-compare -DTWSEARCH_VERSION=v0.6.4-50-gdc1ec4e -DUSE_PTHREADS -DHAVE_FFSLL src/cpp/coset.cpp -o build/cpp/coset.o
    src/cpp/coset.cpp: In function ‘int cosetcallback(setval, const std::vector<int>&, int, int)’:
    src/cpp/coset.cpp:111:11: warning: array subscript 1 is outside array bounds of ‘setval [1]’ [-Warray-bounds]
      111 |     domove(pd, pos, moves[i], *(&pos + 1));
          |     ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    src/cpp/coset.cpp:98:26: note: at offset 8 into object ‘pos’ of size 8
       98 | int cosetcallback(setval pos, const vector<int> &moves, int d, int id) {
          |                   ~~~~~~~^~~
    ...
    g++ -O3 -Wextra -Wall -pedantic -std=c++17 -g -Wsign-compare -o build/bin/twsearch build/cpp/antipode.o build/cpp/calcsymm.o build/cpp/canon.o build/cpp/cmdlineops.o build/cpp/filtermoves.o build/cpp/findalgo.o build/cpp/generatingset.o build/cpp/god.o build/cpp/index.o build/cpp/parsemoves.o build/cpp/prunetable.o build/cpp/puzdef.o build/cpp/readksolve.o build/cpp/solve.o build/cpp/test.o build/cpp/threads.o build/cpp/twsearch.o build/cpp/util.o build/cpp/workchunks.o build/cpp/rotations.o build/cpp/orderedgs.o build/cpp/ffi/ffi_api.o build/cpp/ffi/wasm_api.o build/cpp/city.o build/cpp/coset.o build/cpp/descsets.o build/cpp/ordertree.o build/cpp/unrotate.o build/cpp/shorten.o -lpthread

After
======

    $ make build
    ...
    src/cpp/coset.cpp: In function ‘int cosetcallback(setval, const std::vector<int>&, int, int)’:
    src/cpp/coset.cpp:111:11: error: array subscript 1 is outside array bounds of ‘setval [1]’ [-Werror=array-bounds]
      111 |     domove(pd, pos, moves[i], *(&pos + 1));
          |     ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    src/cpp/coset.cpp:98:26: note: at offset 8 into object ‘pos’ of size 8
       98 | int cosetcallback(setval pos, const vector<int> &moves, int d, int id) {
          |                   ~~~~~~~^~~
    cc1plus: all warnings being treated as errors
    make: *** [Makefile:110: build/cpp/coset.o] Error 1
@jfly
Copy link
Member Author

jfly commented Nov 6, 2023

Strange. CI failed against cpp-windows, but not plain old cpp. Maybe we use a different version of gcc than I do.

I also see CI littered with a bunch of these scary looking logs:

g++ -I./src/cpp/cityhash/src -c -O3 -Wextra -Werror -Wall -pedantic -std=c++17 -g -Wsign-compare -DTWSEARCH_VERSION= -DUSE_PTHREADS -DHAVE_FFSLL src/cpp/util.cpp -o build/cpp/util.o
fatal: No names found, cannot describe anything.

I'll look into this later today.

@lgarron
Copy link
Member

lgarron commented Nov 6, 2023

How strict do we want to be in this repo?

I'm generally a fan of fairly strict code checking, but unlike some other ecosystems it's not straightforward to pin the checks in CI. As you note, even getting consistent behaviour across OSes is a task.

I also see CI littered with a bunch of these scary looking logs:

g++ -I./src/cpp/cityhash/src -c -O3 -Wextra -Werror -Wall -pedantic -std=c++17 -g -Wsign-compare -DTWSEARCH_VERSION= -DUSE_PTHREADS -DHAVE_FFSLL src/cpp/util.cpp -o build/cpp/util.o
fatal: No names found, cannot describe anything.

I'll look into this later today.

That would be this: #9

@jfly
Copy link
Member Author

jfly commented Nov 7, 2023

Closing this in favor of 89ead3c.

We can decide some other time if we want to go as extreme as -Werror.

@jfly jfly closed this Nov 7, 2023
@jfly jfly deleted the werror branch November 7, 2023 18:03
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