WIP: MSVC: lower C4457 warning level from 4 to 3 #105608

Germano Cavalcante · 2023-03-09T16:50:48+01:00

2023-03-09 16:50:48 +01:00

This commit lowers the warning level for C44571, which occurs when a
local variable hides a function parameter with the same name.

The corresponding diagnostic options for GCC and Clang is -Wshadow,
which is curretly enabled for Blender.

By lowering the warning level for C4457, we align the warnings with
other compilers and avoid unnecessary noise.

Note that in MSVC, the warning only checks the name, whereas the
corresponding one in other compilers apparently checks the
[name + type] match. Then some warnings popped up and the code needed
to be edited.

This commit lowers the warning level for C44571, which occurs when a local variable hides a function parameter with the same name. The corresponding diagnostic options for GCC and Clang is `-Wshadow`, which is curretly enabled for Blender. By lowering the warning level for C4457, we align the warnings with other compilers and avoid unnecessary noise. Note that in MSVC, the warning only checks the name, whereas the corresponding one in other compilers apparently checks the [name + type] match. Then some warnings popped up and the code needed to be edited.

Germano Cavalcante added 1 commit 2023-03-09 16:50:53 +01:00

28a8e55d2a MSVC: lower C4457 warning level from 4 to 3

This commit lowers the warning level for C44571, which occurs when a
local variable hides a function parameter with the same name.

The corresponding diagnostic options for GCC and Clang is `-Wshadow`,
which is curretly enabled for Blender.

By lowering the warning level for C4457, we align the warnings with
other compilers and avoid unnecessary noise.

Note that in MSVC, the warning only checks the name, whereas the
corresponding one in other compilers apparently checks the
[name + type] match. Then some warnings popped up and the code needed
to be edited.

Germano Cavalcante requested review from Ray molenkamp 2023-03-09 16:51:11 +01:00

Ray molenkamp commented

2023-03-09 17:05:07 +01:00

Little weirded out by some of the actual code changes fixing the shadowed variables, we're doing this to get close to the GCC/Clang set of warnings, and we end up fixing a bunch of stuff they clearly let slip. The windows specific code files , fair enough that's to be expected but why isn't gcc warning on any of the blenkernel changes?

I'm gonna need to take a closer look there, hope you're not in a rush with this.

Little weirded out by some of the actual code changes fixing the shadowed variables, we're doing this to get close to the GCC/Clang set of warnings, and we end up fixing a bunch of stuff they clearly let slip. The windows specific code files , fair enough that's to be expected but why isn't gcc warning on any of the blenkernel changes? I'm gonna need to take a closer look there, hope you're not in a rush with this.

Germano Cavalcante commented

2023-03-09 17:19:13 +01:00

I also find it strange.
I'll do some tests with GCC to try to understand.
It is strange for example that in GCC const IndexRange range is different from IndexRange range.

I also find it strange. I'll do some tests with GCC to try to understand. It is strange for example that in GCC `const IndexRange range` is different from `IndexRange range`.

Evan Wilson commented

2023-03-11 11:02:24 +01:00

why isn't gcc warning on any of the blenkernel changes?

CTRL-F shad and spamming the 'show addition lines of code for context' arrow reveals 3 occurrences of shadow.

One in macros.cmake.


  if(CMAKE_COMPILER_IS_GNUCC)
    remove_cc_flag(
    ...
      "-Wshadow"
    ...    
          )

Two in CMakeLists.txt

add_check_c_compiler_flag(C_WARNINGS C_WARN_SHADOW -Wshadow)

All the source files this pull changes end in .cc or .cpp

Perhaps adding add_check_cxx_compiler_flag(CXX_WARNINGS CXX_WARN_SHADOW -Wshadow) (by someone in a different pull request) would be prudent given the codebase's recent language shifting to C++.

> why isn't gcc warning on any of the blenkernel changes? CTRL-F `shad` and spamming the '*show addition lines of code for context*' arrow reveals 3 occurrences of `shadow`. One in `macros.cmake`. ```macro(remove_strict_flags) if(CMAKE_COMPILER_IS_GNUCC) remove_cc_flag( ... "-Wshadow" ... ) ``` Two in `CMakeLists.txt` ``` add_check_c_compiler_flag(C_WARNINGS C_WARN_SHADOW -Wshadow) ``` All the source files this pull changes end in `.cc` or `.cpp` Perhaps adding `add_check_cxx_compiler_flag(CXX_WARNINGS CXX_WARN_SHADOW -Wshadow)` (by someone in a different pull request) would be prudent given the codebase's recent language shifting to C++.

Germano Cavalcante commented

2023-03-14 14:38:41 +01:00

Thanks for the info @EAW !

The code in this PR proposed to add the corresponding flag in MSVC through:

set(_WARNINGS
     ...
     "/w34457"  # declaration of 'identifier' hides function parameter
     ...

and

remove_cc_flag(/w34457) # Restore warn C4457 (declaration hides parameter) back to w4

but at the time I didn't notice that the flag was only set for C in GNUCC (and not C++).

I also noticed now that the Clang compiler doesn't even have the -Wshadow flag set!

So this PR makes MSVC more restrictive, and doesn't match current GCC and Clang flags.

The ideal seems to be, for now, to add the flag only for C.

Thanks for the info @EAW ! The code in this PR proposed to add the corresponding flag in MSVC through: ``` set(_WARNINGS ... "/w34457" # declaration of 'identifier' hides function parameter ... ``` and ``` remove_cc_flag(/w34457) # Restore warn C4457 (declaration hides parameter) back to w4 ``` but at the time I didn't notice that the flag was only set for C in GNUCC (and not C++). I also noticed now that the `Clang` compiler doesn't even have the `-Wshadow` flag set! So this PR makes MSVC more restrictive, and doesn't match current GCC and Clang flags. The ideal seems to be, for now, to add the flag only for C.

Germano Cavalcante changed title from ~~MSVC: lower C4457 warning level from 4 to 3~~ to WIP: MSVC: lower C4457 warning level from 4 to 3

2023-03-14 14:39:00 +01:00

This pull request has changes conflicting with the target branch.

build_files/cmake/macros.cmake
intern/ghost/intern/GHOST_WindowWin32.cpp
source/blender/blenkernel/intern/curves_utils.cc
source/blender/blenkernel/intern/geometry_set_instances.cc
source/blender/editors/util/ed_viewer_path.cc
source/blender/nodes/intern/geometry_nodes_log.cc

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u w34457:mano-wii-w34457

git checkout mano-wii-w34457

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: MSVC: lower C4457 warning level from 4 to 3 #105608

Checkout