[core] Turn off -wshadow for tests#56030
Conversation
There was a problem hiding this comment.
Code Review
This pull request aims to disable the -Wshadow compiler warning for tests. It correctly modifies the Bazel build configuration to create a separate set of compiler options for tests (COPTS_TESTS) without this flag. Additionally, it includes a number of valuable fixes for existing shadowing warnings in various test files, which improves code quality.
However, there's a small oversight in the Bazel configuration: the ray_cc_test function is not updated to use the new COPTS_TESTS variable, so tests will still be compiled with -Wshadow. My review includes a comment with a suggestion to fix this. Once that's addressed, this PR will be in great shape.
| COPTS = COPTS_TESTS + select({ | ||
| "//conditions:default": ["-Wshadow"], | ||
| }) |
There was a problem hiding this comment.
While it's correct to add -Wshadow back to COPTS for regular library and binary builds, the ray_cc_test function (defined on line 146) also uses COPTS. This means tests will still be compiled with -Wshadow, which contradicts the goal of this PR.
Please consider changing ray_cc_test to use COPTS_TESTS to disable the warning for tests.
2d3ca59 to
684c8ce
Compare
Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: Masahiro Tanaka <mtanaka@anyscale.com>
Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: Masahiro Tanaka <mtanaka@anyscale.com>
Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: Gang Zhao <gang@gang-JQ62HD2C37.local>
Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: sampan <sampan@anyscale.com>
Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: jugalshah291 <shah.jugal291@gmail.com>
Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: yenhong.wong <yenhong.wong@grabtaxi.com>
Signed-off-by: dayshah <dhyey2019@gmail.com> Signed-off-by: Douglas Strodtman <douglas@anyscale.com>
Signed-off-by: dayshah <dhyey2019@gmail.com>
There's like 13 cpp tests failing to build right now with errors all over because of the -Wshadow we recently added.
Fixing some shadowing and then turning off -Wshadow for the tests.