Build most of Baldr with -Werror #3885
Conversation
|
Thank you for the cleanup and happy new year! I'll review this tomorrow but one thing I noticed is that your changelog entry should be at the bottom of the subsection it's in not at the top. And I may have seen some whitespace discrepencies in the cmakelists.txt changes I'll look closer tomorrow |
| // --<-- --<-- | ||
| // | ||
| // given p (last_node_id) and c (node_id), return n if there is such a node. | ||
| /* |
There was a problem hiding this comment.
can you also revert this change. its best to avoid whitespace-only changes and instead only make changes that fix an issue. unless there is some change that im not detecting here other than the commenting type?
There was a problem hiding this comment.
This generated a warning (and therefore error) on Ubuntu 20.04.5 on ARM:
/home/ubuntu/valhalla/src/baldr/merge.cc:147:3: error: multi-line comment [-Werror=comment]
147 | // / \ e3 / \ e2 / \
| ^
cc1plus: all warnings being treated as errors
make[3]: *** [src/baldr/CMakeFiles/valhalla-baldr.dir/build.make:466: src/baldr/CMakeFiles/valhalla-baldr.dir/merge.cc.o] Error 1
There was a problem hiding this comment.
With GCC 9.4.0:
$ gcc --version
gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
There was a problem hiding this comment.
oh man multiline comment as an error... that is a new level of pedantic..
There was a problem hiding this comment.
ok i guess that comes from -Wall which we didnt have before we just had -Werror. looking at the various namespaces in the project its clear that we havent yet decided what level of warning is desirable for us, some are just -Werror some add specific ones some add -Wextra as you have here. I think for now we can let it as you have it but at some point we should decide what level or warning should be treated as error and commit to that across the project. i have a feeling i'll regret this suggestion though as i tend to hate overly perscriptive compilers and linters and pretty much everyone else likes the opposite 😄
There was a problem hiding this comment.
Yeah I copied and pasted the CMake infrastructure changes from (I think) Mjolnir. I think the sources/sources_with_warnings structure is good and I would probably advocate for standardizing on a common structure and arguments at the top level
|
Thanks for the reviews and happy new year @kevinkreiser and @nilsnolde |
No description provided.